Sc-8652: Handle Expired Certificates in Certman #972
Conversation
DanielSollis
force-pushed
the
sc-8652
branch
from
4632fe4 to
f43f1e8
Compare
pkg/gds/certman/certs.go
Outdated
| @@ -824,6 +835,10 @@ vaspsLoop: | |||
| } | |||
| } | |||
|
|
|||
| func RetrieveCertID(identityCert *pb.Certificate) string { | |||
| return fmt.Sprintf("%X", identityCert.SerialNumber) | |||
| } | |||
There was a problem hiding this comment.
I do like this helper, however I think we can make it a bit more useful by moving it to the models package (pkg/models/v1) so other packages can use it. Then, let's replace all instances where we are encoding the serial number with this helper method so that we can standardize that a bit more (basically just search the repo for fmt.Sprintf("%X).
Also since we're moving it to a different package can we change the name of the method to GetCertID or something along those lines to better match the naming conventions there?
There was a problem hiding this comment.
That's a good idea!
pkg/gds/certman/certs.go
Outdated
| continue vaspsLoop | ||
| } | ||
| cert.Status = models.CertificateState_EXPIRED | ||
| c.db.UpdateCert(cert) |
There was a problem hiding this comment.
We need to handle any errors returned by UpdateCert here, my first instinct is that we don't need to bail out of the loop or anything but I think we should at least log the error. What do you think?
There was a problem hiding this comment.
Fixed! I hadn't notice UpdateCert returning an error, my bad!
| s.updateVaspIdentityCert(charlieVASP, 6) | ||
| s.updateVaspIdentityCert(hotelVASP, 1) |
There was a problem hiding this comment.
Was this test failing? I don't understand why this line is necessary now.
There was a problem hiding this comment.
Ya, hotelVASP's cert is expired so it was triggering the <= 7 days before expiration case by default, but now since we have a <= 0 days before expiration case it was triggering that and throwing off the expected emails and causing the test to fail, so I added this line to update it's cert expiration.
There was a problem hiding this comment.
That makes complete sense, thanks.
pkg/gds/certman/certs_test.go
Outdated
| defer s.fixtures.LoadReferenceFixtures() | ||
| require := s.Require() | ||
|
|
||
| // setup the datastore to contain the modified charlieVASP |
There was a problem hiding this comment.
| // setup the datastore to contain the modified charlieVASP | |
| // Setup the datastore to contain the modified hotelVASP |
|
|
||
| cert, err = s.db.RetrieveCert(certID) | ||
| require.NoError(err) | ||
| require.Equal(cert.Status, models.CertificateState_EXPIRED) |
There was a problem hiding this comment.
So hotelVASP has an expired certificate by default?
There was a problem hiding this comment.
yep, see the above comment.
Scope of changes
Updates the Certman reissuance loop to handle the case where the certificate has expired.
Type of change
Acceptance criteria
Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible
Author checklist
Reviewer(s) checklist