Skip to content

Commit

Permalink
PKI: add denylist Key and URL (#2329)
Browse files Browse the repository at this point in the history
* add denylist Key and URL

* fix tests

* workaround for missing certificate during outbound call

* fix race condition
  • Loading branch information
gerardsn authored Jul 12, 2023
1 parent 9275a2a commit 0e39a29
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 25 deletions.
5 changes: 5 additions & 0 deletions network/transport/grpc/connection_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,11 @@ func (s *grpcConnectionManager) revalidatePeers() {
s.lastCertificateValidation.Store(&now)
s.connections.forEach(func(conn Connection) {
peerCert := conn.Peer().Certificate
if peerCert == nil {
// This can happen when the denylist is updated while the node is trying to set up an outbound connection.
// See https://github.com/nuts-foundation/nuts-node/issues/2333
return
}
if nowFunc().After(peerCert.NotAfter) {
log.Logger().WithError(errors.New("certificate expired while in use")).WithFields(conn.Peer().ToFields()).Info("Disconnected peer")
conn.disconnect()
Expand Down
2 changes: 0 additions & 2 deletions pki/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ func FlagSet() *pflag.FlagSet {
// Flags for denylist features
flagSet.Int("pki.maxupdatefailhours", defs.MaxUpdateFailHours, "Maximum number of hours that a denylist update can fail")
flagSet.Bool("pki.softfail", defs.Softfail, "Do not reject certificates if their revocation status cannot be established when softfail is true")
// TODO: Choose a default trusted signer key
flagSet.String("pki.denylist.trustedsigner", defs.Denylist.TrustedSigner, "Ed25519 public key (in PEM format) of the trusted signer for denylists")
// TODO: Choose a default denylist URL
flagSet.String("pki.denylist.url", defs.Denylist.URL, "URL of PKI denylist (set to empty string to disable)")

// Changing these config values is not recommended, and they are expected to almost always be the same value, so
Expand Down
4 changes: 2 additions & 2 deletions pki/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ package pki
func DefaultConfig() Config {
return Config{
Denylist: DenylistConfig{
URL: "",
TrustedSigner: "",
URL: "https://cdn.jsdelivr.net/gh/nuts-foundation/denylist@main/denylist/denylist.jws",
TrustedSigner: "-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEAmKjcSOrKOJR2cYd6UNbemNeusvjs930Y4nCIZ1R2zCI=\n-----END PUBLIC KEY-----",
},
MaxUpdateFailHours: 4,
Softfail: true,
Expand Down
4 changes: 2 additions & 2 deletions pki/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ func Test_DefaultConfig(t *testing.T) {
assert.Equal(t, 4, cfg.MaxUpdateFailHours)
assert.True(t, cfg.Softfail)
require.NotNil(t, cfg.Denylist)
assert.Empty(t, cfg.Denylist.TrustedSigner)
assert.Empty(t, cfg.Denylist.URL)
assert.NotEmpty(t, cfg.Denylist.TrustedSigner)
assert.NotEmpty(t, cfg.Denylist.URL)
}
26 changes: 13 additions & 13 deletions pki/denylist.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type denylistImpl struct {
trustedKey jwk.Key

// lastUpdated contains the time the certificate was last updated
lastUpdated time.Time
lastUpdated atomic.Pointer[time.Time]

// subscribers for denylist updates
subscribers []func()
Expand All @@ -68,13 +68,14 @@ type denylistEntry struct {

// NewDenylist creates a denylist with the specified configuration
func NewDenylist(config DenylistConfig) (Denylist, error) {
// initialize defaults
dl := &denylistImpl{url: config.URL}
dl.lastUpdated.Store(&time.Time{})

// "Disable" (operate in a NOP mode) the denylist when the URL is empty
if config.URL == "" {
if dl.url == "" {
// Return the new denylist and a nil error
return &denylistImpl{
trustedKey: nil,
url: "",
}, nil
return dl, nil
}

// Convert any literal '\n' in the PEM to an actual newline character
Expand All @@ -87,10 +88,8 @@ func NewDenylist(config DenylistConfig) (Denylist, error) {
}

// Return the new denylist and a nil error
return &denylistImpl{
trustedKey: key,
url: config.URL,
}, nil
dl.trustedKey = key
return dl, nil
}

// ValidateCert checks for the issuer and serialNumber in the denylist, returning nil when the cert is permitted
Expand All @@ -107,7 +106,7 @@ func (b *denylistImpl) ValidateCert(cert *x509.Certificate) error {
thumbprint := certKeyJWKThumbprint(cert)

// If the denylist has not yet been downloaded, do so now
if b.lastUpdated.IsZero() {
if b.lastUpdated.Load().IsZero() {
// Trigger an update of the denylist
if err := b.Update(); err != nil {
// If the denylist download failed then log a message about it
Expand Down Expand Up @@ -159,7 +158,7 @@ func (b *denylistImpl) ValidateCert(cert *x509.Certificate) error {
}

func (b *denylistImpl) LastUpdated() time.Time {
return b.lastUpdated
return *b.lastUpdated.Load()
}

func (b *denylistImpl) URL() string {
Expand Down Expand Up @@ -198,7 +197,8 @@ func (b *denylistImpl) Update() error {
b.entries.Store(&entries)

// Track when the denylist was last updated
b.lastUpdated = time.Now()
now := nowFunc()
b.lastUpdated.Store(&now)

// Log when the denylist is updated
logger().Debug("Denylist updated successfully")
Expand Down
15 changes: 12 additions & 3 deletions pki/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,25 @@ package pki
import (
"crypto/x509"
"testing"
"time"
)

// TestConfig is the same as DefaultConfig without a denylist URL set.
func TestConfig(t *testing.T) Config {
return Config{
Denylist: DenylistConfig{},
MaxUpdateFailHours: 4,
Softfail: true,
}
}

// SetNewDenylistWithCert sets a new Denylist on the Validator and adds the certificate.
// This is useful in integrations tests etc.
func SetNewDenylistWithCert(t *testing.T, val Validator, cert *x509.Certificate) {
dl := &denylistImpl{
url: "some-url",
lastUpdated: time.Now(),
url: "some-url",
}
now := nowFunc()
dl.lastUpdated.Store(&now)
dl.entries.Store(&[]denylistEntry{
{
Issuer: cert.Issuer.String(),
Expand Down
6 changes: 3 additions & 3 deletions pki/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestValidator_Start(t *testing.T) {
require.NoError(t, err)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
val, err := newValidatorWithHTTPClient(DefaultConfig(), newClient())
val, err := newValidatorWithHTTPClient(TestConfig(t), newClient())
require.NoError(t, err)
require.NoError(t, val.AddTruststore(store.Certificates()))

Expand Down Expand Up @@ -197,7 +197,7 @@ func TestValidator_AddTruststore(t *testing.T) {
require.NoError(t, err)

t.Run("ok", func(t *testing.T) {
val, err := newValidator(DefaultConfig())
val, err := newValidator(TestConfig(t))
require.NoError(t, err)

err = val.AddTruststore(store.Certificates())
Expand Down Expand Up @@ -228,7 +228,7 @@ func TestValidator_SubscribeDenied(t *testing.T) {
mockDenylist := NewMockDenylist(gomock.NewController(t))
mockDenylist.EXPECT().Subscribe(gomock.Any())

val, err := newValidator(DefaultConfig())
val, err := newValidator(TestConfig(t))
require.NoError(t, err)
val.denylist = mockDenylist

Expand Down

0 comments on commit 0e39a29

Please sign in to comment.