-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add escape hatch for GODEBUG=x509negativeserial #6371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add escape hatch for GODEBUG=x509negativeserial #6371
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Alano Terblanche <[email protected]>
2f37b74 to
7d7a7aa
Compare
| switch m := meta.Metadata.(type) { | ||
| case DockerContext: | ||
| config, ok = m.AdditionalFields[fieldName] | ||
| if !ok { | ||
| return | ||
| } | ||
| case map[string]any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we could even consider just checking for map[string]any - some of the "must match this type" handling with contexts is a bit over-engineered (or at least, ultimately, it's a map[string]any that could be unmarshaled to a custom type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tested it and it didn't work 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, yeah, I was afraid of that. I think it's because the code already casts the metadata to a specific type. Definitely not critical, so something we can look at later if we want to remove some of those abstractions. Here's (probably) why that happens; the context is set up with a DefaultContextResolver;
Lines 270 to 272 in b3cd9d4
| Resolver: func() (*DefaultContext, error) { | |
| return ResolveDefaultContext(cli.options, *cli.contextStoreConfig) | |
| }, |
Which I think would convert things;
cli/cli/command/defaultcontextstore.go
Lines 56 to 57 in b3cd9d4
| // ResolveDefaultContext creates a Metadata for the current CLI invocation parameters | |
| func ResolveDefaultContext(opts *cliflags.ClientOptions, config store.Config) (*DefaultContext, error) { |
cli/command/cli.go
Outdated
| func (cli *DockerCli) setAllowNegativex509() { | ||
| cn := cli.CurrentContext() | ||
| meta, err := cli.ContextStore().GetMetadata(cn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli.CurrentContext() is just returning the value as-is; no special handling of synchronisation/locking. Given that this is an internal method, we can peel away those layers and just access the field;
Lines 429 to 431 in e069ded
| func (cli *DockerCli) CurrentContext() string { | |
| return cli.currentContext | |
| } |
Once we start peeling away those layers, then everything looks to be initialized in the function we're calling this method from, so perhaps the meta could be just passed in as argument (but I haven't looked too closely at that);
Lines 267 to 273 in 7d7a7aa
| cli.currentContext = resolveContextName(cli.options, cli.configFile) | |
| cli.contextStore = &ContextStoreWithDefault{ | |
| Store: store.New(config.ContextStoreDir(), *cli.contextStoreConfig), | |
| Resolver: func() (*DefaultContext, error) { | |
| return ResolveDefaultContext(cli.options, *cli.contextStoreConfig) | |
| }, | |
| } |
The only part that slightly concerns me is if we would be initialising more because of the call to the context store, which may now be called before we even consider making API (or networking) calls. Perhaps not a big issue, but at least from the looks of it, it looks like the "default' (i.e., "no context used") case is still not entirely a "no-op", so we could short-circuit it and return early;
cli/cli/command/defaultcontextstore.go
Lines 135 to 142 in e069ded
| func (s *ContextStoreWithDefault) GetMetadata(name string) (store.Metadata, error) { | |
| if name == DefaultContextName { | |
| defaultContext, err := s.Resolver() | |
| if err != nil { | |
| return store.Metadata{}, err | |
| } | |
| return defaultContext.Meta, nil | |
| } |
|
Oh; bike-shedding, but I'm considering perhaps we should namespace the option (also in light of having good conventions for these in future if we want to start using the metadata more), e.g. |
|
Wondering if we should make it more explicit that "Metadata": {
"GODEBUG": {
"allowx509negativeserialdonotuse": "1"
}
}, |
Signed-off-by: Alano Terblanche <[email protected]>
Signed-off-by: Alano Terblanche <[email protected]>
| // set the GODEBUG environment variable with whatever was in the context | ||
| _ = os.Setenv(fieldName, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if that would override what we currently set in our go.mod on buildx repo: https://github.com/docker/buildx/blob/67218bef58f934f3f19f66346891ea4fbdc1ad4e/go.mod#L187-L188
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it merges the key-value pairs that are specified by the GODEBUG environment variable with the go.mod ones.
When a GODEBUG setting is not listed in the environment variable, its value is derived from three sources: the defaults for the Go toolchain used to build the program, amended to match the Go version listed in go.mod, and then overridden by explicit //go:debug lines in the program.
e.g.
GODEBUG=http2client=0
---
go.mod
--
godebug (
http2server=0
)
will result in
godebug (
http2server=0
http2client=0
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we specify GODEBUG=winsymlink=1 then it will override the value set inside buildx's go.mod. (but that's already possible without this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok looks good thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for checking! I was curious indeed how those options would interact with a go.mod having it set.
Curious; does that go.mod option take effect when building in GOPATH mode? Or does that ignore go.mod entirely?
Signed-off-by: Alano Terblanche <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an escape hatch mechanism to allow setting the GODEBUG environment variable through Docker context metadata, specifically to support the deprecated x509negativeserial=1 flag for handling certificates with negative serial numbers.
- Introduces a
setGoDebugfunction that readsGODEBUGvalues from context metadata and sets the environment variable - Integrates the function into the CLI initialization process
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cli/command/cli.go | Implements the main setGoDebug function and integrates it into CLI initialization |
| cli/command/cli_test.go | Adds unit tests for the new setGoDebug functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Alano Terblanche <[email protected]>
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
but I need to jump out for some hours, so perhaps @vvoland could double check, and open a cherry-pick 🤗
vvoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
woops, still wanted to squash my commits 🙈 |
|
Yeah, I missed it this time; well, no harm done! |
- What I did
Negative serial number certificates are deprecated and Go has also since deprecated them. To use them with Go you need to pass in the GODEBUG=x509negativeserial=1 environment variable.
This PR allows setting the value inside
docker contextinstead. This should then propagate environment variables to underlying plugins (such as buildx).- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)