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.

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

Embed Apple root and intermediate certificates into quill binary #34

Merged
merged 4 commits into from Apr 11, 2023

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented Apr 8, 2023

This PR makes the following changes to quill:

  • Fails signing if certificate verification fails (which now requires the full chain). Previously verification would be skipped with a warning when the full chain was not found. Now there is an error indicating the chain is missing. This can be turned off with the QUILL_SIGN_FAIL_WITHOUT_FULL_CHAIN env var set to false.
  • Embeds all of the root and intermediate certificates found from https://www.apple.com/certificateauthority/ via a go generate step. These certificates are checked into the repo. Note: no automation updates these certificates, they will need to be updated manually and checked in with make update-apple-certs
  • Adds new quill embedded-certificates command to enumerate the Apple certificates embedded in the quill binary
  • renames the pem package to pki (more accurate, especially with the current additions)
  • splits the pki packages into load for loading material from sources and certchain for operating on a cert store or chain.

TODO:

  • add test that shows failure with single cert during signing and another test that sets QUILL_SIGN_FAIL_WITHOUT_FULL_CHAIN to false and shows the previous test steps pass.
  • refactor the embedded store behind an interface to make testing easier -- add unit tests for pki.FindRemainingChainCerts to show that the embedded lookup works as expected with test-fixture certs.
  • update README.md to show that the attach-chain step is no longer necessary (but also show it's necessary for non-apple roots being used)
  • maybe rename the quill apple-certs command? [renamed to embedded-certificates]

Closes #8
Closes #16

@wagoodman wagoodman self-assigned this Apr 8, 2023
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman wagoodman force-pushed the embed-apple-certs branch 2 times, most recently from fdaa786 to 1b61f82 Compare April 11, 2023 16:14
@wagoodman wagoodman marked this pull request as ready for review April 11, 2023 16:15
@wagoodman wagoodman requested a review from a team April 11, 2023 16:15
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM

quill/pki/load/p12.go Outdated Show resolved Hide resolved
quill/pki/load/private_key.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman wagoodman enabled auto-merge (squash) April 11, 2023 17:42
@wagoodman wagoodman merged commit ef4c04d into main Apr 11, 2023
5 checks passed
@wagoodman wagoodman deleted the embed-apple-certs branch April 11, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants