fix(certs): update checkAndRotate to use current durations #4800

jaellio · 2022-06-08T21:28:37Z

Description:

Updates the checkAndRotate method to update certs with the current
validity duration specified in the MeshConfig for their respective
cert type. Adds a certType to the certificate struct that is used
to lookup validity duration when calling IssuingCertificate.
Updates shouldRotate to check if the cert needs to be rotated due
to an in-progress root certificate rotation. GetFromCache will
return nil if the cert is not found in the cache, expiring, or
needs to be rotated because of a root cert rotation.

Resolves #4810

Testing done:

Verified expiring certs are re-issued with the current serviceCertValidityDuration as defined in the OSM MeshConfig. Currently, expiring service certs are only re-issued with the original serviceCertValidityDuration.
- Verified the bug on the main branch by running the osm demo with the serviceCertValidityDuration set to 5m. After OSM install, I patched the MeshConfig and updated the validity duration to 4m. Expiring certs were reissued using the original 5m validity duration.
- Verified the bug is no longer present with this change by following the same steps as above
CI

Affected area:

Functional Area
New Functionality	[x]
Certificate Management	[x]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project? no
- Did you notify the maintainers and provide attribution?
Is this a breaking change? no
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

codecov-commenter · 2022-06-09T21:05:21Z

Codecov Report

Merging #4800 (ee0d641) into main (8da8732) will decrease coverage by 0.06%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##             main    #4800      +/-   ##
==========================================
- Coverage   69.47%   69.41%   -0.07%     
==========================================
  Files         218      218              
  Lines       15766    15819      +53     
==========================================
+ Hits        10954    10980      +26     
- Misses       4760     4787      +27     
  Partials       52       52

Flag	Coverage Δ
unittests	`69.41% <73.68%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/osm-controller/osm-controller.go	`14.46% <0.00%> (ø)`
pkg/certificate/certificate.go	`94.87% <ø> (-0.97%)`	⬇️
pkg/certificate/fake_manager.go	`69.11% <0.00%> (-2.10%)`	⬇️
pkg/certificate/options.go	`33.33% <ø> (-31.38%)`	⬇️
pkg/certificate/providers/types.go	`0.00% <ø> (ø)`
pkg/certificate/types.go	`100.00% <ø> (ø)`
pkg/configurator/methods.go	`65.38% <0.00%> (-6.65%)`	⬇️
pkg/configurator/mock_client_generated.go	`0.00% <0.00%> (ø)`
pkg/certificate/manager.go	`79.25% <89.10%> (+0.79%)`	⬆️
pkg/certificate/providers/config.go	`76.82% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8da8732...ee0d641. Read the comment docs.

pkg/certificate/manager.go

pkg/certificate/providers/tresor/fake/fake.go

steeling · 2022-06-13T17:23:28Z

pkg/certificate/manager.go

+			continue
+		}
+		if newCert != cert {
+			// Certificate was rotated


So there's a race condition here where IssueCertificate could have triggered a rotation between the call to list, and the check here.

One thought is we can add the logic to issuecertificate for messaging on a rotation if we need to rotate.

Moved publishing to the cert pub-sub to IssueCertificate to ensure a message is published whenever a certificate is rotated (including outside of checkAndRotate).

steeling · 2022-06-13T17:25:34Z

pkg/certificate/manager.go

-			cert = cert.newMergedWithRoot(validatingIssuer.CertificateAuthority)
-		}
+	validityDuration := m.getValidityDurationForCertType(ct)
+	cert, err = signingIssuer.IssueCertificate(cn, validityDuration)


with my above comment, there's still a race condition if issue gets called twice in a row.

singleflight is a package meant to fix exactly that issue, so we may want to leverage that here

Updated IssueCertificate so all calls to issue certificates are included in group.Do.

pkg/certificate/manager.go

shashankram · 2022-06-16T20:01:28Z

pkg/configurator/methods.go

@@ -162,6 +165,22 @@ func (c *Client) GetServiceCertValidityPeriod() time.Duration {
 	return validityDuration
 }

+// GetIngressGatewayCertValidityPeriod returns the validity duration for ingress gateway certificates, and a default in case of unspecified or invalid duration
+func (c *Client) GetIngressGatewayCertValidityPeriod() time.Duration {


What's the use case to use a default if unspecified? I'd rather make this field required in the CRD spec vs assuming user intent.

If GetIngressGatewaCertValidityPeriod() is somehow called when the ingressGateway field isn't specified should it return an error? Currently, If the ingressGateway ValidityDuration field is updated and the duration is invalid the ingressGatewayCert won't be updated. On rotation of the ingressGateway cert we'll attempt to get the duration from the meshConfig, but it will be invalid. Do you think it makes sense to return a default duration in this case?

If an ingressGateway is specified in the MeshConfig then the duration is required. We could specify a default instead of making it required? I guess ideally we would validate the MeshConfig and prevent invalid durations before they are applied.

If it's a required property, why do we need a default? Is there a default to simplify the code path?

If the ingressGateway field is removed/unset, the behavior as implemented in pkg/ingress/gateway.go is to call ReleaseCertificate(). So rotation should not happen for such a scenario?

Yes. Additionally,

On rotation of the ingressGateway cert we'll attempt to get the duration from the meshConfig, but it will be invalid.

I am wondering if we ever want to use a default during rotation for the ingress cert. I don't see a need to rotate the ingress gateway cert when the state of the gateway cert spec is deemed invalid. Would it be easier to prevent a misconfig?

This is not a blocking comment. I can understand if there are a few corner cases being guarded against through defensive programming here.

Also I would imagine, if the updated value is invalid, we should continue to use the older value encoded in the previous cert and not a default?

If the ingress gateway cert's validity duration is set in the meshConfig to an invalid duration the cert will not be reissued but the invalid duration will remain set in the meshConfig. When the ingress gateway cert expires should the previous validity duration be used? Prior to my change, the service cert validity duration would have been used for the ingress gateway cert on expiration. I could add a validity duration field to the Certificate struct or I could add validation for the MeshConfig, but I think that is beyond the scope of this PR.

@shashankram Could this be addressed in a follow-up PR? I think this could be addressed when we add validation for the MeshConfig. I am also happy to make the changes to include the validity duration as a field in Certificate or make those changes in a follow-up PR as well.

Yes, sure. This comment is not blocking in nature.

jaellio · 2022-06-17T22:51:20Z

pkg/certificate/options.go

-}
-
-// WithValidityPeriod modifies the expiration on a newly issued certificate.
-func WithValidityPeriod(validityPeriod time.Duration) IssueOption {


@steeling I removed this because I didn't want to make the cert type/ validity duration optional and assume a default. With my current design for cert rotation and issuing it is required that I have a cert type or something similar to determine whether to call getServiceCertValidityDuration, getIngressGatewayCertValidityDuration, or use the OSM internal cert duration. Do you have any suggestions for how to incorporate the validity period options with my changes or take a different approach? Thanks!!

pkg/certificate/manager.go

steeling · 2022-06-20T20:48:12Z

pkg/certificate/manager.go

@@ -15,10 +17,13 @@ import (
 	"github.com/openservicemesh/osm/pkg/messaging"
 )

+var group singleflight.Group


i'd consider scoping this to the certmanager, in case there are ever multiple

Good idea. I think I might leave this as-is for this PR unless this is a blocking comment.

I would say it’s a bug not to, since if 2 Cert manager objects are created the cache would not be populated correctly.

It’s a 1-2 line fix, so I would push for this in this pr

pkg/certificate/manager.go

steeling · 2022-06-20T20:51:59Z

pkg/certificate/manager.go

+func (m *Manager) issueCertificate(prefix string, ct CertType, opts ...IssueOption) (*Certificate, error) {
+	var rotate bool
+	cert := m.getFromCache(prefix) // Don't call this while holding the lock
+	if cert != nil {


nit: put the nil check in should rotate to reduce nesting

I think I prefer the nesting the call to shouldRotate. It makes sense to only call shouldRotate when the cert is not nil. Happy to make the change if you think moving the nil check to shouldRotate improves readability.

Updates the checkAndRotate method to update certs with the current validity duration specified in the MeshConfig for their respective cert type. Adds a certType to the certificate struct that is used to lookup validity duration when calling IssuingCertificates. Updates shouldRotate to check if the cert needs to be rotated due to an in progress root certificate rotation. GetFromCache will return nil if the cert is not found in the cache, expiring, or needs to be rotated because of a root cert rotation. Signed-off-by: jaellio <jaellio@microsoft.com>

Signed-off-by: jaellio <jaellio@microsoft.com>

jaellio force-pushed the addCertTypes branch 7 times, most recently from 1e833a9 to 76ad808 Compare June 9, 2022 20:51

jaellio marked this pull request as ready for review June 9, 2022 20:51

jaellio requested review from shashankram, snehachhabria, nojnhuh, draychev and trstringer as code owners June 9, 2022 20:51

jaellio changed the title ~~fix(certs): updates checkAndRotate to use current durations~~ fix(certs): update checkAndRotate to use current durations Jun 9, 2022

jaellio mentioned this pull request Jun 13, 2022

Automated Root Certificate rotation in OSM #4502

Closed

30 tasks

jaellio force-pushed the addCertTypes branch 3 times, most recently from 39990d4 to 378ce46 Compare June 14, 2022 17:43

nojnhuh reviewed Jun 14, 2022

View reviewed changes

pkg/certificate/manager.go Outdated Show resolved Hide resolved

pkg/certificate/providers/tresor/fake/fake.go Outdated Show resolved Hide resolved

steeling suggested changes Jun 14, 2022

View reviewed changes

jaellio force-pushed the addCertTypes branch 4 times, most recently from 0730a64 to 637fd7a Compare June 14, 2022 23:39

shashankram reviewed Jun 16, 2022

View reviewed changes

jaellio force-pushed the addCertTypes branch 2 times, most recently from e84a5fe to 85e1c60 Compare June 17, 2022 22:38

jaellio commented Jun 17, 2022

View reviewed changes

shashankram approved these changes Jun 17, 2022

View reviewed changes

nojnhuh approved these changes Jun 20, 2022

View reviewed changes

jaellio force-pushed the addCertTypes branch from 8307c87 to 5794543 Compare June 20, 2022 18:45

steeling suggested changes Jun 20, 2022

View reviewed changes

jaellio added 2 commits June 21, 2022 13:21

Address PR comments on singleflight

49df5dd

Signed-off-by: jaellio <jaellio@microsoft.com>

jaellio force-pushed the addCertTypes branch from 7b9221f to 49df5dd Compare June 21, 2022 20:21

Add group to Manager

ee0d641

Signed-off-by: jaellio <jaellio@microsoft.com>

jaellio force-pushed the addCertTypes branch from 9f477f9 to ee0d641 Compare June 21, 2022 23:13

steeling approved these changes Jun 22, 2022

View reviewed changes

jaellio merged commit 28b3238 into openservicemesh:main Jun 22, 2022

fix(certs): update checkAndRotate to use current durations #4800

fix(certs): update checkAndRotate to use current durations #4800

Uh oh!

Conversation

jaellio commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaellio Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaellio commented Jun 8, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

jaellio Jun 17, 2022 •

edited

Loading