Credential template builder package #3832
Conversation
azdagron
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
This package consolidates all of the CA and SVID template building logic into one spot where the Credential Composers can be applied. The package does perform some input validation of parameters but does unfortunately does not perform validation against alterations returned by credential composers. This is because it is not possible to know how the extra extensions will impact the certificates until after they have passed through CreateCertificate, and we don't want to sign a credential after each CredentialComposer invocation. As such, the validation will be left to the server CA layer after signing has taken place. Signed-off-by: Andrew Harding <[email protected]>
azdagron
force-pushed
the
credtemplate-pkg
branch
from
ea277a1 to
11cb6f2
Compare
Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Andrew Harding <[email protected]>
|
|
||
| // It's a bit gross, but SPIRE has historically signed downstream X509CA's with the X509-SVID ttl, so | ||
| // let's override the NotBefore/NotAfter fields set by buildX509CATemplate. | ||
| tmpl.NotBefore, tmpl.NotAfter = b.computeX509SVIDLifetime(params.ParentChain, params.TTL) |
There was a problem hiding this comment.
TTL was set using buildX509CATemplate may we avoid that? in favor to this one?
There was a problem hiding this comment.
The way I structured the code is that buildX509CATemplate applies the TTL as defined by the X509CATTL. The downstream CA should have always been minted based on that TTL, but has not been. This code that recomputes the TTL based on the X509SVIDTTL is a backwards compatibility hack and an exception. I don't see much value muddying buildX509CATemplate with additional complication around TTL setting just to accommodate this one-off hack.
| // The first DNS name is also added as the CN by default. This happens | ||
| // even if the subject is provided explicitly in the params for backwards | ||
| // compatibility. Ideally we wouldn't do override the subject in this | ||
| // case. It is still overridable via the credential composers however. |
There was a problem hiding this comment.
this worry me a little, so..
if someone set DNS as part of a composer, we will have SVIDs where subject is no longer set by the first DNS,
even we can have cases were we set a DNS (so subject will change) but a composer can remove DNSs or change order and we'll have unexpected results...
I'm not saying it is an issue, but we'll need to add documentation so people notice about this can be affected by a composer.
There was a problem hiding this comment.
I agree this needs to be clearly documented... do you have an alternate suggestion on how this is implemented that retains backwards compatibility?
In my mind, the CredentialComposer should be allowed to do anything, as long as it produces a SPIFFE spec conforming certificate.
There was a problem hiding this comment.
agree, I think we must allow them to update it, so I dont see this as an issue but a feature,
but what worry me is than someone may be confused about possible results
Signed-off-by: Andrew Harding <[email protected]>
This package consolidates all of the CA and SVID template building logic into one spot where the Credential Composers can be applied. The package does perform some input validation of parameters but does unfortunately does not perform validation against alterations returned by credential composers. This is because it is not possible to know how the extra extensions will impact the certificates until after they have passed through CreateCertificate, and we don't want to sign a credential after each CredentialComposer invocation. As such, the validation will be left to the server CA layer after signing has taken place. Signed-off-by: Andrew Harding <[email protected]>
This package consolidates all of the CA and SVID template building logic into one spot where the Credential Composers can be applied.
The package does perform some input validation of parameters but does unfortunately does not perform validation against alterations returned by credential composers. This is because it is not possible to know how the extra extensions will impact the certificates until after they have passed through CreateCertificate, and we don't want to sign a credential after each CredentialComposer invocation.
As such, the validation will be left to the server CA layer after signing has taken place.