Skip to content
Open
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
20 changes: 16 additions & 4 deletions std/security/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,17 +213,29 @@ func revocationRecordName(cert ndn.Data) (enc.Name, bool) {
return recordName.Append(certName.At(-2)), true
}

func isRevocationRecordName(name enc.Name) bool {
// CertNameFromRevocationRecordName returns the certificate name encoded by a
// revocation record name.
func CertNameFromRevocationRecordName(name enc.Name) (enc.Name, error) {
name = stripImplicitDigest(name)
if len(name) < 6 || name.At(-1).Typ != enc.TypeGenericNameComponent || !name.At(-2).IsVersion() {
return false
if len(name) < 6 ||
!name.At(-5).IsGeneric("REVOKE") ||
!name.At(-2).IsVersion() ||
!name.At(-1).Equal(name.At(-3)) {
return nil, fmt.Errorf("invalid revocation record name: %s", name)
}

certName := make(enc.Name, len(name)-1)
copy(certName, name.Prefix(-1))
certName[len(name)-5] = enc.NewGenericComponent("KEY")

_, err := GetIdentityFromCertName(certName)
if _, err := GetIdentityFromCertName(certName); err != nil {
return nil, fmt.Errorf("invalid revocation record name %s: %w", name, err)
}
return certName, nil
}

func isRevocationRecordName(name enc.Name) bool {
_, err := CertNameFromRevocationRecordName(name)
return err == nil
}

Expand Down
20 changes: 20 additions & 0 deletions std/security/certificate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,26 @@ func TestCertificateRevocationRecord(t *testing.T) {
require.True(t, ok)
require.Equal(t, ndn.ContentTypeKey, contentType)
require.NotEmpty(t, recordData.Content())

certName, err := sec.CertNameFromRevocationRecordName(recordData.Name())
require.NoError(t, err)
require.True(t, aliceCert.Name().Equal(certName))
}

func TestCertNameFromRevocationRecordNameRejectsMalformedNames(t *testing.T) {
tests := []string{
"/REVOKE/key/issuer/v=1/issuer",
"/test/alice/REVOKE/key/issuer/not-version/issuer",
"/test/alice/REVOKE/key/issuer/v=1/v=2",
"/test/alice/KEY/key/issuer/v=1/issuer",
}

for _, value := range tests {
t.Run(value, func(t *testing.T) {
_, err := sec.CertNameFromRevocationRecordName(tu.NoErr(enc.NameFromStr(value)))
require.Error(t, err)
})
}
}

func revocationTestIssuer(t *testing.T) enc.Component {
Expand Down
47 changes: 46 additions & 1 deletion std/security/trust_config.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package security

import (
"bytes"
"crypto/sha256"
"fmt"
"sync"

Expand Down Expand Up @@ -107,13 +109,55 @@ func (tc *TrustConfig) InsertRevoke(wire enc.Wire) error {
if !isRevocationRecordName(data.Name()) {
return fmt.Errorf("not a revocation record name: %s", data.Name())
}
record, err := revocationtlv.ParseRevocationRecord(enc.NewWireView(data.Content()), false)
if err != nil {
return fmt.Errorf("failed to parse revocation record content: %w", err)
}
if len(record.PublicKeyHash) != sha256.Size {
return fmt.Errorf("invalid revocation public-key hash length: %d", len(record.PublicKeyHash))
}

tc.mutex.Lock()
defer tc.mutex.Unlock()
if current, _ := tc.keychain.Store().Get(data.Name(), false); len(current) > 0 {
if bytes.Equal(current, wire.Join()) {
return nil
}
return fmt.Errorf("conflicting revocation record already stored: %s", data.Name())
}
Comment on lines +122 to +127

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could there be a use case for one certificate getting revoked multiple times?

@tianyuan129 ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added this check to make InsertRevoke idempotent under replay. Receiving the same wire again can happen through sync or reconnection, so that case returns nil. A different packet with the same record name is rejected instead of silently overwriting the stored record.

Since the record name is deterministic for the exact certificate, this currently gives us first-write-wins behavior. A second revocation could still be useful to correct the reason or NotBefore, so this check would be too strict if updates are meant to be supported. In that case we should define whether the later record replaces the earlier one or gets a distinct/versioned name. I'll wait for Tianyuan's view before changing the behavior.

// HACK: revocation records live in keychain.Store() until we have a dedicated store.
return tc.keychain.Store().Put(data.Name(), wire.Join())
}

// CheckRevoke returns the stored revocation record for cert. A nil record
// means the certificate has no locally stored revocation.
func (tc *TrustConfig) CheckRevoke(cert ndn.Data) (*revocationtlv.RevocationRecord, error) {
if cert == nil {
return nil, fmt.Errorf("certificate is nil")
}
recordName, ok := revocationRecordName(cert)
if !ok {
return nil, fmt.Errorf("invalid certificate name: %s", cert.Name())
}

tc.mutex.RLock()
wire, err := tc.keychain.Store().Get(recordName, false)
tc.mutex.RUnlock()
if err != nil || len(wire) == 0 {
return nil, nil
}

data, _, err := spec.Spec{}.ReadData(enc.NewBufferView(wire))
if err != nil {
return nil, fmt.Errorf("failed to parse stored revocation record: %w", err)
}
record, err := revocationtlv.ParseRevocationRecord(enc.NewWireView(data.Content()), false)
if err != nil {
return nil, fmt.Errorf("failed to parse stored revocation content: %w", err)
}
return record, nil
}

// SetSchema atomically replaces the trust schema.
func (tc *TrustConfig) SetSchema(schema ndn.TrustSchema) {
if schema == nil {
Expand Down Expand Up @@ -198,7 +242,8 @@ func (tc *TrustConfig) Validate(args TrustConfigValidateArgs) {

// Bail if the data is a cert and is not fresh
if t, ok := args.Data.ContentType().Get(); ok && t == ndn.ContentTypeKey {
if !args.IgnoreValidity.GetOr(false) && CertIsExpired(args.Data) {
if !isRevocationRecordName(args.Data.Name()) &&
!args.IgnoreValidity.GetOr(false) && CertIsExpired(args.Data) {
args.Callback(false, fmt.Errorf("certificate is expired: %s", args.Data.Name()))
return
}
Expand Down
20 changes: 20 additions & 0 deletions std/security/trust_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package security_test

import (
"crypto/elliptic"
"crypto/sha256"
_ "embed"
"fmt"
"math"
Expand Down Expand Up @@ -1092,9 +1093,28 @@ func TestTrustConfigRevocation(t *testing.T) {

require.Error(t, trust.InsertRevoke(nil))
require.Error(t, trust.InsertRevoke(aliceCertWire))
_, err = trust.CheckRevoke(nil)
require.Error(t, err)

recordWire := makeRevocationRecordWire(t, aliceCertData, rootSigner)
require.NoError(t, trust.InsertRevoke(recordWire))
require.NoError(t, trust.InsertRevoke(recordWire))
conflictingRecord := tu.NoErr(sec.RevokeCert(sec.RevokeCertArgs{
Cert: aliceCertData,
Signer: rootSigner,
Timestamp: optional.Some(time.Now().Add(time.Minute)),
}))
require.Error(t, trust.InsertRevoke(conflictingRecord))
stored, err := trust.CheckRevoke(aliceCertData)
require.NoError(t, err)
require.NotNil(t, stored)
require.Equal(t, sha256.Sum256(aliceCertData.Content().Join()), [32]byte(stored.PublicKeyHash))

reloadedTrust, err := sec.NewTrustConfig(keychain, schema, []enc.Name{rootCertData.Name()})
require.NoError(t, err)
stored, err = reloadedTrust.CheckRevoke(aliceCertData)
require.NoError(t, err)
require.NotNil(t, stored)

recordData, _, err := spec.Spec{}.ReadData(enc.NewWireView(recordWire))
require.NoError(t, err)
Expand Down
Loading