feat: add inline cert provider#601
feat: add inline cert provider#601susanshi merged 9 commits intonotaryproject:mainfrom cse-labs:inline-cert-provider
Conversation
|
@susanshi Went ahead and took a stab at implementing this. It's functional and tested as-is. It seems relatively straightforward - I've not used this outside of my own dev box yet Wanted to get some design feedback/thoughts before moving it out of draft This currently specifies "one PEM per CertificateStore" - which could be a cert chain if you are using root/intermediate CA's. I wasn't sure whether to keep it that way or make it multivalued since you can add multiple entries under Ex: |
susanshi
left a comment
susanshi
left a comment
There was a problem hiding this comment.
Hi Noel, thanks so much for the turnaround. A new inline provider is exactly what we ve discussed, it looks great! i have left some refactoring comments, let me know what you think! thanks!
Codecov ReportBase: 42.07% // Head: 42.03% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
- Coverage 42.07% 42.03% -0.05%
==========================================
Files 53 55 +2
Lines 3194 3214 +20
==========================================
+ Hits 1344 1351 +7
- Misses 1703 1715 +12
- Partials 147 148 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
noelbundick-msft
requested review from
akashsinghal,
binbin-li,
luisdlp and
toddysm
as code owners
| type CertificateProvider interface { | ||
| // Returns an array of certificates based on certificate properties defined in attrib map | ||
| GetCertificatesContent(ctx context.Context, attrib map[string]string) ([]types.Certificate, error) | ||
| GetCertificates(ctx context.Context, attrib map[string]string) ([]*x509.Certificate, error) |
There was a problem hiding this comment.
One of the future items is to enable Certificate providers in the cosign verifier path. It looks like cosign load a ECDSA key, will this interface work with cosign?
We want to keep this interface generic so it works for all verifiers that requires a validation cert. We haven't started exploring the integration with Cosign yet, appreciate your insight here :)
There was a problem hiding this comment.
Will look into this and get back w/ some info :)
There was a problem hiding this comment.
What I've found as a complete cosign newbie:
- We're not actually using this interface right now- was just trying to get things lined up consistently to prepare for implementing the factory in the future - and to try to use common interfaces vs the Key Vault-specific
types.Certificateone here - This won't work as-is with our cosign code that exists today - it expects a plain public key rather than a certificate. I'd be wary of doing "not certificate things" in the "certificate provider". Might need a new "public key provider", or change the name to something else in the future to cover both use cases?
- Our linked usage of cosign functions points to functions labeled "TODO: move this to an internal package" and only seem to be used to parse a key. I wonder if there are more suitable functions in the
sigstore/signaturepackage directly?
If cosign users have and typically uses ECDSA certificates, it seems easy enough to use. This test case loads in a cert w/ an ECDSA key, and then uses it to load up a sigstore verifier:
func TestUseX509WithSigstore(t *testing.T) {
certString := "-----BEGIN CERTIFICATE-----\nMIIB+TCCAZ+gAwIBAgIUNK/+de2dV3kCMVOGj+X6mdyO6A8wCgYIKoZIzj0EAwIw\nUjELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAldBMRAwDgYDVQQHDAdSZWRtb25kMREw\nDwYDVQQKDAhUZXN0Q2VydDERMA8GA1UEAwwIdGVzdGNlcnQwHhcNMjMwMjA4MTkz\nNjAwWhcNMjQwMjA4MTkzNjAwWjBSMQswCQYDVQQGEwJVUzELMAkGA1UECAwCV0Ex\nEDAOBgNVBAcMB1JlZG1vbmQxETAPBgNVBAoMCFRlc3RDZXJ0MREwDwYDVQQDDAh0\nZXN0Y2VydDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABDLn+GbbWXyXcVuQ/PFP\nmC1f8FiXK7IBCATvOZXxQLJnU8B55mo1BTZn8yBzT1TJA4XfQktNrZYHIWwJkeFh\njgSjUzBRMB0GA1UdDgQWBBQ4wBIh6ub86EFcRvPss6dEPPd7IDAfBgNVHSMEGDAW\ngBQ4wBIh6ub86EFcRvPss6dEPPd7IDAPBgNVHRMBAf8EBTADAQH/MAoGCCqGSM49\nBAMCA0gAMEUCIQCaEZDL399k2fnwGQCluc32Xk8Dugsucv+niWZCIBtA4AIgDYBZ\n2HVNJ/29vqxAHG4mzNXH4uBZj9DvJpmYbzUvlOk=\n-----END CERTIFICATE-----\n"
c1 := []byte(certString)
// this works for certificates, but wouldn't work for plain public keys
certs, err := DecodeCertificates(c1)
if err != nil {
t.Fatalf(err.Error())
}
// I havent verified anything with this, but this creates an ECDSAVerifier w/o errors
verifier, err := signature.LoadVerifier(certs[0].PublicKey, crypto.SHA256)
if err != nil {
t.Fatalf(err.Error())
}
fmt.Printf("verifier: %+v", verifier)
}That said, if cosign typically uses public keys directly - this approach (and therefore involving x509) probably wouldn't make much sense
I've seen some cosign refactoring issues/PR's, particularly around making it work with authenticated registries. Would it make sense to land this feature and then include "how do we handle certs/keys in a consistent way?" in the scope of the design for that improvement?
There was a problem hiding this comment.
I think it's fair to say that our current cosign implementation with respect to handling public keys and interacting with registries is not optimal. I'd be in favor of a general overhaul of how cosign plugin and integration with ORAS store is done. This one is going to require a fair bit of research and work as far as I can tell...
There was a problem hiding this comment.
Thanks so much for looking into this. I think you make a good point, certificate providers should return certificates. I think we can go ahead with the interface update.
"TODO: move this to an internal package" this message is worrying, we probably will need to revisit the cosign verifier soon.
susanshi
left a comment
susanshi
left a comment
There was a problem hiding this comment.
thanks for adding the e2e test! LGTM
|
Hi @noelbundick-msft , once this is rebased, this PR is good to merge. |
4 participants
Description
What this PR does / why we need it:
Users need a way to refer to public certs (where they don't have the private key) during signature verification
Ratify currently has an
"azurekeyvault"certificate provider. This PR adds an"inline"cert provider where a PEM-format certificate (chain) can be directly specified. This is a big improvement over the current experience, where users have to (ab)use theratifyTestCertHelm param and/or reverse engineer the chart to volume mount a ConfigMap/Secret.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #576
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
CertificateStorecontaining my CA public certificate, configuringverifier-notaryto , which was able to verify a notary v2 signature that was created by a leaf certificate. ex:Checklist: