-
Notifications
You must be signed in to change notification settings - Fork 448
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
Add SCEP provisioner decrypter #1414
Conversation
Instead of relying on an intermediate `scep.Service` struct, initialize the `scep.Authority` directly. This removes one redundant layer of indirection.
When changing the SCEP configuration it is possible that one or both of the decrypter configurations required are not available or have been provided in a way that's not usable for actual SCEP requests. Instead of failing hard when provisioners are loaded, which could result in the CA not starting properly, this type of problematic configuration errors will now be handled at usage time instead.
Instead of the old method that redacted sensitive information by overriding the value of the property and changing it back to the original, the API now uses a model specifically meant for API responses. This prevents potential race conditions. This may be iterated on a bit so that we don't need to rely on the [provisioner.Interface] interface, which requires the API model to implement unnecessary methods.
…tep/certificates into herman/scep-provisioner-decrypter
Forgot to save the latest version...
Add SCEP issuance notification webhook
…initialization Change SCEP authority initialization
This commit uses the same types for the fields in the provisioner.SCEP type and the "redacted" models.SCEP.
Fix redacted types in SCEP provisioner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this for now, but there are some changes we might want to consider before creating a new release.
DecrypterCertificate: []byte(redacted), | ||
DecrypterKeyPEM: []byte(redacted), | ||
DecrypterKeyURI: redacted, | ||
DecrypterKeyPassword: []byte(redacted), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this in #1554
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to use a separate model for the public API response, so that *** REDACTED ***
would show up instead of the base64 encoding of that. So we may want to revisit this, and make the CLI use the "public" model instead. But there's probably more to be discussed around that, since I only did this for this single provisioner/endpoint.
if a.validateSCEP { | ||
// validate the SCEP authority | ||
if err := scepAuthority.Validate(); err != nil { | ||
a.initLogf("failed validating SCEP authority: %v", err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal, but for now, it is ok.
DecrypterCertificate []byte `json:"decrypterCertificate,omitempty"` | ||
DecrypterKeyPEM []byte `json:"decrypterKeyPEM,omitempty"` | ||
DecrypterKeyURI string `json:"decrypterKey,omitempty"` | ||
DecrypterKeyPassword []byte `json:"decrypterKeyPassword,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have users using a release candidate where this is a string
. I would prefer to use []byte
for this. But custom Password
type that can have either base64 or plain passwords can be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People may have been using it, but those changes were only in a (draft) PR until now, so I don't think we need to spend a lot of time on keeping this compatible. I know of at least one user who tested this with the decrypterKey
with cloudkms
, and that doesn't need the password.
case o.Signer == nil: | ||
return errors.New("no signer available for SCEP authority") | ||
case o.SignerCert == nil: | ||
return errors.New("no signer certificate available for SCEP authority") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok for now, but when these fields are always configured at the provisioner level, these fields are not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thought just occurred to me: we probably still need this, and need some way to explicitly not rely on the provisioner specific signer in specific situations. I'll follow up with a discussion / PR for that.
} | ||
|
||
if decryptionKeyURI := s.DecrypterKeyURI; len(decryptionKeyURI) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for now, but we might want to consider using only one option.
This PR introduces support for provisioner specific decrypters for SCEP. This allows a SCEP provisioner to be configured with an RSA key used for decrypting SCEP requests independent of the authority intermediate signing key. In turn, this means that one can then add SCEP provisioners to CAs that have been configured with e.g. an ECDSA intermediate signing key.
Changes from smallstep/linkedca#55 need to be merged first.
To configure a SCEP provisioner using the CLI, changes from smallstep/cli#950 are needed.
In
ca.json
, the configuration would look similar to the below examples:The
decrypterCertificate
should contain the base64 encoding of a PEM formatted certificate.This has been tested with
softkms
andcloudkms
, so far. One limitation with usingsoftkms
is that the password currently needs to be provided in the configuration; it can't be read from a file or the environment at this time.In addition to the functional changes, this PR also simplifies how the SCEP authority is initalized and validated, as well as how the SCEP provisioner is passed by context.