Skip to content

hack: add script to regenerate certificates and update test-certs#42389

Merged
cpuguy83 merged 2 commits intomoby:masterfrom
thaJeztah:update_certs
May 20, 2021
Merged

hack: add script to regenerate certificates and update test-certs#42389
cpuguy83 merged 2 commits intomoby:masterfrom
thaJeztah:update_certs

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 18, 2021

relates to #40353

Certificates were originally added in c000cb6 (#3068), but did not include a script to generate them. Current versions of Go expect certificates to use SAN instead of Common Name fields, so updating the script to include those;

x509: certificate relies on legacy Common Name field, use SANs or temporarily
enable Common Name matching with GODEBUG=x509ignoreCN=0

Some fields were updated to be a bit more descriptive (instead of "replaceme"), and the -text option was used to include a human-readable variant of the content.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah added 2 commits May 18, 2021 09:43
@thaJeztah
hack: add script to regenerate certificates
2fea30f 
Certificates were originally added in c000cb6,
but did not include a script to generate them. Current versions of Go expect
certificates to use SAN instead of Common Name fields, so updating the script
to include those;

    x509: certificate relies on legacy Common Name field, use SANs or temporarily
    enable Common Name matching with GODEBUG=x509ignoreCN=0

Some fields were updated to be a bit more descriptive (instead of "replaceme"),
and the `-text` option was used to include a human-readable variant of the
content.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Update test certificates
fe54215 
Updates the certificates to account for current versions of Go expecting
SANs to be used instead of the Common Name field:

    FAIL: s390x.integration.plugin.authz TestAuthZPluginTLS (0.53s)
    [2020-07-26T09:36:58.638Z]     authz_plugin_test.go:132: assertion failed:
        error is not nil: error during connect: Get "https://localhost:4271/v1.41/version":
        x509: certificate relies on legacy Common Name field, use SANs or temporarily enable Common Name matching with GODEBUG=x509ignoreCN=0

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added this to the 21.xx milestone May 18, 2021
@thaJeztah thaJeztah requested a review from tianon as a code owner May 18, 2021 07:46
@thaJeztah thaJeztah mentioned this pull request May 18, 2021
Merged
20 tasks
@thaJeztah
Copy link
Member Author

thaJeztah commented May 18, 2021

failure on arm is a known flaky test; #42357

[2021-05-18T08:05:56.917Z] --- FAIL: TestDaemonHostGatewayIP (5.39s)
[2021-05-18T08:05:56.917Z]     daemon_linux_test.go:161: [db2862c8d069f] failed to start daemon with arguments [--data-root /go/src/github.com/docker/docker/bundles/test-integration/TestDaemonHostGatewayIP/db2862c8d069f/root --exec-root /tmp/dxr/db2862c8d069f --pidfile /go/src/github.com/docker/docker/bundles/test-integration/TestDaemonHostGatewayIP/db2862c8d069f/docker.pid --userland-proxy=true --containerd-namespace db2862c8d069f --containerd-plugins-namespace db2862c8d069fp --containerd /var/run/docker/containerd/containerd.sock --host unix:///tmp/docker-integration/db2862c8d069f.sock --debug --storage-driver overlay2 --host-gateway-ip=6.7.8.9] : [db2862c8d069f] daemon exited during startup: exit status 1

tianon
tianon approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly the level of complexity and line count I expected when I saw the PR title 😭

LG(enough)TM

@thaJeztah
Copy link
Member Author

thaJeztah commented May 19, 2021 •
edited
Loading

Exactly the level of complexity and line count I expected when I saw the PR title 😭

Oh man, I went through multiple iterations; at first tried https://github.com/dmcgowan/quicktls (which is super handy), and even contributed a PR to it to add SAN support (dmcgowan/quicktls#3) but that didn't set the "subject" (CN) that was used by the Authz plugin. Tried https://github.com/FiloSottile/mkcert, which was recommended as an easy tool (and created by one of Go's security people, so seemed like a good choice), however that one also didn't offer the ability to set the CN (fields were filled by some "dummy" data). It also refuses to use * as SAN (which could be "correct", or not.. ? openssh accepts it, but TBH not sure if it's valid.. tests pass, so 🤷‍♂️)

So... time to dive into the openssh man pages, which was "down the rabbit hole". openssh design really looks to be "security is important to us, so we make this as complicated as can be for you. We hope you mess things up, so that security teams won't be out of a job!".

  • learned that openssh req can either produce a csr or an actual certificate (if -x509 is set) 🤯 (great! so we can do it all in a single step... right?)
  • when using openssh req -x509, the -addext option can be used to set x509 options (awesome! I can pass the options on the command-line, that way I don't need to create some config file 🥳)
  • So I just pass the -CA and -CAkey to sign... wait.. those flags are not present?
  • ok, nevermind, two steps it is then; create the CSR with all the options set, and then create / sign the certificate in the second step with openssh x509
  • ahm.. right, so while x509 options can be included in a CSR, they are not inherited by openssh x509
  • no worries! I'll just move the -addext flag to the openssh x509 step. oh.. right... that one doesn't support passing them on the command line
  • back to creating a "config" file then 😞

@tianon
Copy link
Member

tianon commented May 19, 2021

Yepppp, it's a mess (slightly better in OpenSSL 3.0+ IIRC, but still pretty ugly). I can definitely relate. 😅

cpuguy83
cpuguy83 approved these changes May 20, 2021
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit 8858ecb into moby:master May 20, 2021
@thaJeztah thaJeztah deleted the update_certs branch May 20, 2021 19:43
@thaJeztah thaJeztah mentioned this pull request Jul 15, 2021
Merged
@thaJeztah thaJeztah mentioned this pull request Apr 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

area/testing process/cherry-picked status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @tianon @cpuguy83