Support Custom CA for certificate issuing #438
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skiptomyliu
previously approved these changes
Oct 15, 2024
Yueren-Wang
reviewed
Oct 15, 2024
|
|
||
| def _load_rootca_certificate(self, ca_json): | ||
| if "rootcrt" not in ca_json or not ca_json["rootcrt"]: | ||
| logger.warning("Custom CA has no root CA certificate provided") |
There was a problem hiding this comment.
maybe adding ca id to enrich the log for debugging purpose?
logger.warning("Custom CA %s has no root CA certificate provided", ca_env)
Yueren-Wang
reviewed
Oct 15, 2024
|
|
||
| acceptable_validity = min(validity, self.settings["max_validity_days"]) | ||
| builder = builder.not_valid_after( | ||
| datetime.now(timezone.utc) + timedelta(days=acceptable_validity) |
There was a problem hiding this comment.
call datetime.now twice at 129 and 133. considering consolidate them into 1?
Yueren-Wang
reviewed
Oct 15, 2024
| active_ca = [ | ||
| ca | ||
| for ca in CUSTOM_CERTIFICATE_AUTHORITIES[ca_env] | ||
| if validator.validate(ca) and ca["kid"] == active_ca_id |
There was a problem hiding this comment.
if validator.validate(ca) return False, will we still log Custom CA %s has no active keys?
Yueren-Wang
reviewed
Oct 15, 2024
| # Get the certificate in PEM format | ||
| intermediate_ca_pem = self.encode_certificate(self.ca_certificate) | ||
| root_ca_pem = self.encode_certificate(self.root_ca_certificate) | ||
| return intermediate_ca_pem + root_ca_pem |
There was a problem hiding this comment.
self.root_ca_certificate can be None? if so intermediate_ca_pem + root_ca_pem will throw TypeError
Yueren-Wang
previously approved these changes
Oct 15, 2024
meng-han
dismissed stale reviews from Yueren-Wang and skiptomyliu
via
1958e28
|
/offload |
Yueren-Wang
approved these changes
Oct 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
previously we have one giant class certificatemanager which has a certificatauthority class which is basically ACM PCA.
Now we want to add support for custom ca, without impacting existing contracts/apis on
/v1/certificates prefix. So a new base class is created so that both acm pca and custom ca can inherit from.other changes are mostly tests and settings along side this.