Skip to content
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

Sc-8652: Handle Expired Certificates in Certman #972

Merged
merged 9 commits into from
Jan 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/gds/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ func (s *Admin) ListVASPs(c *gin.Context) {

// Add certificate serial number if it exists
if vasp.IdentityCertificate != nil {
snippet.CertificateSerial = fmt.Sprintf("%X", vasp.IdentityCertificate.SerialNumber)
snippet.CertificateSerial = models.GetCertID(vasp.IdentityCertificate)
snippet.CertificateIssued = vasp.IdentityCertificate.NotBefore
snippet.CertificateExpiration = vasp.IdentityCertificate.NotAfter
}
Expand Down Expand Up @@ -1036,7 +1036,7 @@ func (s *Admin) prepareVASPDetail(vasp *pb.VASP, log zerolog.Logger) (out *admin
log.Warn().Msg("could not parse identity certificate serial number from vasp json")
return nil, errors.New("could not parse identity certificate serial number from vasp json")
}
out.VASP["identity_certificate"].(map[string]interface{})["serial_number"] = fmt.Sprintf("%X", vasp.IdentityCertificate.SerialNumber)
out.VASP["identity_certificate"].(map[string]interface{})["serial_number"] = models.GetCertID(vasp.IdentityCertificate)
}

// Convert the signing certificate serial numbers to capital hex encoded strings
Expand All @@ -1045,7 +1045,7 @@ func (s *Admin) prepareVASPDetail(vasp *pb.VASP, log zerolog.Logger) (out *admin
log.Warn().Msg("could not parse signing certificate serial number from vasp json")
return nil, errors.New("could not parse signing certificate serial number from vasp json")
}
out.VASP["signing_certificates"].([]interface{})[i].(map[string]interface{})["serial_number"] = fmt.Sprintf("%X", cert.SerialNumber)
out.VASP["signing_certificates"].([]interface{})[i].(map[string]interface{})["serial_number"] = models.GetCertID(cert)
}

return out, nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/gds/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func (s *gdsTestSuite) TestListVASPs() {
RegisteredDirectory: "trisatest.net",
VerifiedOn: hotel.VerifiedOn,
VerificationStatus: pb.VerificationState_VERIFIED.String(),
CertificateSerial: fmt.Sprintf("%X", hotel.IdentityCertificate.SerialNumber),
CertificateSerial: models.GetCertID(hotel.IdentityCertificate),
CertificateIssued: hotel.IdentityCertificate.NotBefore,
CertificateExpiration: hotel.IdentityCertificate.NotAfter,
VerifiedContacts: map[string]bool{
Expand Down Expand Up @@ -751,7 +751,7 @@ func (s *gdsTestSuite) TestRetrieveVASP() {
require.Len(actual.VerifiedContacts, 2)

// Verify that the identity certificate serial number was converted to a capital hex encoded string
expectedSerial := fmt.Sprintf("%X", hotel.IdentityCertificate.SerialNumber)
expectedSerial := models.GetCertID(hotel.IdentityCertificate)
actualSerial, ok := actual.VASP["identity_certificate"].(map[string]interface{})["serial_number"]
require.True(ok, "identity_certificate.serial_number not found in VASP json")
require.Equal(expectedSerial, actualSerial)
Expand All @@ -763,7 +763,7 @@ func (s *gdsTestSuite) TestRetrieveVASP() {
for i, cert := range actualCerts {
actualSerial, ok := cert.(map[string]interface{})["serial_number"]
require.True(ok, "signing certificate serial number not found in VASP json")
expectedSerial := fmt.Sprintf("%X", hotel.SigningCertificates[i].SerialNumber)
expectedSerial := models.GetCertID(hotel.SigningCertificates[i])
require.Equal(expectedSerial, actualSerial)
}

Expand Down
19 changes: 16 additions & 3 deletions pkg/gds/certman/certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -729,13 +729,14 @@ vaspsLoop:
}

// Make sure the VASP has an identity certificate to avoid a panic.
if vasp.IdentityCertificate == nil {
identityCert := vasp.IdentityCertificate
if identityCert == nil {
log.Error().Err(err).Str("vasp_id", vasp.Id).Msg("vasp is verified but does not have an identity certificate")
continue vaspsLoop
}

// Calculate the number of days before the VASP's certificate expires.
if expirationDate, err = time.Parse(time.RFC3339, vasp.IdentityCertificate.NotAfter); err != nil {
if expirationDate, err = time.Parse(time.RFC3339, identityCert.NotAfter); err != nil {
log.Error().Err(err).Str("vasp_id", vasp.Id).Msg("could not parse %s's cert reissuance date")
continue vaspsLoop
}
Expand All @@ -751,9 +752,21 @@ vaspsLoop:
reissuanceDate := expirationDate.Add(-time.Hour * 240)
timeBeforeExpiration := time.Until(expirationDate)

// TODO: handle the case where the certificate has expired, we should update the certificate record to the EXPIRED state
// NOTE: This computation returns fractional days rather than rounding up or down to the nearest day.
switch daysBeforeExpiration := timeBeforeExpiration.Hours() / 24; {

// If a certificate has expired, update the certificate record.
case daysBeforeExpiration <= 0:
var cert *models.Certificate
if cert, err = c.db.RetrieveCert(models.GetCertID(identityCert)); err != nil {
log.Error().Err(err).Str("vasp_id", vasp.Id).Msg("could not retrieve expired certificate record")
continue vaspsLoop
}
cert.Status = models.CertificateState_EXPIRED
if err = c.db.UpdateCert(cert); err != nil {
log.Error().Err(err).Str("vasp_id", vasp.Id).Msg("could not update expired certificate record status")
}

// Seven days before expiration, send a cert reissuance reminder to VASP.
// NOTE: the SendContactReissuanceReminder will not send emails more than
// once to a contact.
Expand Down
35 changes: 33 additions & 2 deletions pkg/gds/certman/certs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,10 @@ func (s *certTestSuite) TestCertManagerSevenDayReissuanceReminder() {
require.NoError(err, "could not get hotel VASP")
hotelVASP = s.setupVASP(hotelVASP)

// Call the certman function at 6 days, which will send
// the seven day cert reissuance reminder to echoVASP.
// Call the certman function at 6 days and 1 day, which will send seven day
// cert reissuance reminders to charlieVASP and hotelVASP.
s.updateVaspIdentityCert(charlieVASP, 6)
s.updateVaspIdentityCert(hotelVASP, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this test failing? I don't understand why this line is necessary now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes complete sense, thanks.

callTime := time.Now()
s.certman.HandleCertificateReissuance()

Expand Down Expand Up @@ -312,6 +313,36 @@ func (s *certTestSuite) TestCertManagerSevenDayReissuanceReminder() {
emails.CheckEmails(s.T(), messages)
}

func (s *certTestSuite) TestCertManagerExpiration() {
s.setupCertManager(sectigo.ProfileCipherTraceEE, fixtures.Small)
defer s.teardownCertManager()
defer s.fixtures.LoadReferenceFixtures()
require := s.Require()

// setup the datastore to contain the modified hotelVASP
hotelVASP, err := s.fixtures.GetVASP("hotel")
require.NoError(err, "could not get hotel VASP")
hotelVASP = s.setupVASP(hotelVASP)

certID := models.GetCertID(hotelVASP.IdentityCertificate)
cert := &models.Certificate{
Id: certID,
Status: models.CertificateState_ISSUED,
}
err = s.db.UpdateCert(cert)
require.NoError(err)

// Run the loop again to ensure that emails are not resent to contacts
s.certman.HandleCertificateReissuance()

cert, err = s.db.RetrieveCert(certID)
require.NoError(err)
require.Equal(cert.Status, models.CertificateState_EXPIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So hotelVASP has an expired certificate by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, see the above comment.


// Ensure that emails have been sent with the mock email client.
emails.CheckEmails(s.T(), []*emails.EmailMeta{})
}

func (s *certTestSuite) TestCertManagerReissuance() {
require := s.Require()

Expand Down
7 changes: 6 additions & 1 deletion pkg/models/v1/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ func NewCertificate(vasp *pb.VASP, certRequest *CertificateRequest, data *pb.Cer
}

cert = &Certificate{
Id: fmt.Sprintf("%X", data.SerialNumber), // capital hex encoded serial number to match sectigo
Id: GetCertID(data), // capital hex encoded serial number to match sectigo
Request: certRequest.Id,
Vasp: vasp.Id,
Status: CertificateState_ISSUED,
Expand All @@ -431,6 +431,11 @@ func NewCertificate(vasp *pb.VASP, certRequest *CertificateRequest, data *pb.Cer
return cert, nil
}

// String encode the SerialNumber of a certificate
func GetCertID(identityCert *pb.Certificate) string {
return fmt.Sprintf("%X", identityCert.SerialNumber)
}

// NewCertificateRequest creates and returns a new certificate request to be associated with a VASP.
func NewCertificateRequest(vasp *pb.VASP) (certRequest *CertificateRequest, err error) {
var (
Expand Down