Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use ctlog.config for creating certs, add managecaroots job, tests. #352

Merged
merged 5 commits into from
Sep 21, 2022
Merged

Use ctlog.config for creating certs, add managecaroots job, tests. #352

merged 5 commits into from
Sep 21, 2022

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Sep 14, 2022 •
edited
Loading

Signed-off-by: Ville Aikas vaikas@chainguard.dev

Summary

Addresses but does not fix: https://github.com/sigstore/public-good-instance/issues/524

Release Note

Documentation

@vaikas vaikas changed the title [WIP] Use ctlog.config for creating certs, add managefulcio job, tests. Use ctlog.config for creating certs, add managefulcio job, tests. Sep 15, 2022
@vaikas vaikas enabled auto-merge (squash) September 16, 2022 04:42
haydentherapper
haydentherapper requested changes Sep 19, 2022
View reviewed changes
cmd/ctlog/createctconfig/main.go
if err != nil {
log.Fatalf("Failed to marshal new configuration: %v", err)
}
// Take out the public / private key from the secret since we didn't mess
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unclear on why these are removed from the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, because the reconcilesecret only 'reconciles' the keys that are in this map. We pull them out of the map so that they are never modified by the run of this script.
https://github.com/sigstore/scaffolding/blob/main/pkg/secret/secret.go#L28

cmd/ctlog/createctconfig/main.go Outdated Show resolved Hide resolved
cmd/ctlog/managefulcio/main.go Outdated Show resolved Hide resolved
cmd/ctlog/verifyfulcio/main.go
logging.FromContext(ctx).Fatalf("Failed to marshal fulcio Root cert %s cert: %w", fulcio.String(), err)
}
logging.FromContext(ctx).Infof("Got a root cert for fulcio Root cert %s", fulcio)
// Strip the certificate begin/end marker strings since CTLog doesn't
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this accurate? https://github.com/sigstore/public-good-instance/blob/main/config/ctfe/root.pem shows that we did specify PEM headers. Or is this just how we store them in the ctlog config, and then add headers back later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how they are being served from the logs:

vaikas@villes-mbp scaffolding % curl https://ctfe.sigstore.dev/test/ct/v1/get-roots | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1381  100  1381    0     0   4538      0 --:--:-- --:--:-- --:--:--  4681
{
  "certificates": [
    "MIIB+DCCAX6gAwIBAgITNVkDZoCiofPDsy7dfm6geLbuhzAKBggqhkjOPQQDAzAqMRUwEwYDVQQKEwxzaWdzdG9yZS5kZXYxETAPBgNVBAMTCHNpZ3N0b3JlMB4XDTIxMDMwNzAzMjAyOVoXDTMxMDIyMzAzMjAyOVowKjEVMBMGA1UEChMMc2lnc3RvcmUuZGV2MREwDwYDVQQDEwhzaWdzdG9yZTB2MBAGByqGSM49AgEGBSuBBAAiA2IABLSyA7Ii5k+pNO8ZEWY0ylemWDowOkNa3kL+GZE5Z5GWehL9/A9bRNA3RbrsZ5i0JcastaRL7Sp5fp/jD5dxqc/UdTVnlvS16an+2Yfswe/QuLolRUCrcOE2+2iA5+tzd6NmMGQwDgYDVR0PAQH/BAQDAgEGMBIGA1UdEwEB/wQIMAYBAf8CAQEwHQYDVR0OBBYEFMjFHQBBmiQpMlEk6w2uSu1KBtPsMB8GA1UdIwQYMBaAFMjFHQBBmiQpMlEk6w2uSu1KBtPsMAoGCCqGSM49BAMDA2gAMGUCMH8liWJfMui6vXXBhjDgY4MwslmN/TJxVe/83WrFomwmNf056y1X48F9c4m3a3ozXAIxAKjRay5/aj/jsKKGIkmQatjI8uupHr/+CxFvaJWmpYqNkLDGRU+9orzh5hI2RrcuaQ==",
    "MIIB9zCCAXygAwIBAgIUALZNAPFdxHPwjeDloDwyYChAO/4wCgYIKoZIzj0EAwMwKjEVMBMGA1UEChMMc2lnc3RvcmUuZGV2MREwDwYDVQQDEwhzaWdzdG9yZTAeFw0yMTEwMDcxMzU2NTlaFw0zMTEwMDUxMzU2NThaMCoxFTATBgNVBAoTDHNpZ3N0b3JlLmRldjERMA8GA1UEAxMIc2lnc3RvcmUwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAAT7XeFT4rb3PQGwS4IajtLk3/OlnpgangaBclYpsYBr5i+4ynB07ceb3LP0OIOZdxexX69c5iVuyJRQ+Hz05yi+UF3uBWAlHpiS5sh0+H2GHE7SXrk1EC5m1Tr19L9gg92jYzBhMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBRYwB5fkUWlZql6zJChkyLQKsXF+jAfBgNVHSMEGDAWgBRYwB5fkUWlZql6zJChkyLQKsXF+jAKBggqhkjOPQQDAwNpADBmAjEAj1nHeXZp+13NWBNa+EDsDP8G1WWg1tCMWP/WHPqpaVo0jhsweNFZgSs0eE7wYI4qAjEA2WB9ot98sIkoF3vZYdd3/VtWB5b9TNMea7Ix/stJ5TfcLLeABLE4BNJOsQ4vnBHJ"
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting, this is base64 encoded DER. I guess pem.Decode handles headerless PEM?

cmd/ctlog/verifyfulcio/main.go
}
}
}
// Check that all the expected roots were found.
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, if we're supporting third-party CAs, we would want to check that we've found all fulcio roots, but it might just be a subset of roots in the ctlog config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought we might need that, so thanks for that use case :)

There's the --strict flag that we can then flip to false. For the tests, I wanted to make sure it was a precise match, but thought there would be other use cases.
https://github.com/sigstore/scaffolding/pull/352/files#diff-f3a6ec0f5f1577f486baea8af21fd6f52dc4a009ed193babaef0b758e09eb841R57

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

cmd/ctlog/verifyfulcio/main.go Show resolved Hide resolved
pkg/ctlog/config.go Show resolved Hide resolved
@vaikas vaikas changed the title Use ctlog.config for creating certs, add managefulcio job, tests. Use ctlog.config for creating certs, add managecaroots job, tests. Sep 20, 2022
@vaikas
Copy link
Contributor Author

vaikas commented Sep 20, 2022

Thanks for the review @haydentherapper I think I addressed all your comments, PTAL :)

@vaikas
Use ctlog.config for creating certs, add managefulcio job (for adding…
d7b4726 
…/removing), tests.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas
Add a test that makes sure that onlyexpected Fulcio certificates
35454bf 
are in the CTLog trusted roots.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas
sigh, wrong flag name for fulcio.
5ae4065 
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas
Address PR feedback.
03a022b 
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas
Address PR feedback.
dc1fa04 
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@vaikas vaikas merged commit 1437577 into sigstore:main Sep 21, 2022
@vaikas vaikas deleted the fulcio-key-rotate-tests branch September 21, 2022 19:11
haydentherapper
haydentherapper reviewed Sep 21, 2022
View reviewed changes
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Late LGTM, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haydentherapper haydentherapper haydentherapper requested changes

@priyawadhwa priyawadhwa priyawadhwa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to handle multiple Fulcio certs in the createctconfig.
3 participants
@vaikas @haydentherapper @priyawadhwa