Skip to content

Commit

Permalink
feat: add support for +-5 min clock skew
Browse files Browse the repository at this point in the history
By default PGP library considers any key which was generated "in the
future" as expired without any allowed clock skew. This fixes that, and
adds some tests.

Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
  • Loading branch information
smira committed Nov 7, 2022
1 parent 7b80a50 commit cdb9722
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 12 deletions.
35 changes: 23 additions & 12 deletions pkg/pgp/key.go
Expand Up @@ -16,8 +16,10 @@ import (
pgpcrypto "github.com/ProtonMail/gopenpgp/v2/crypto"
)

// Time-related key settings.
const (
maxAllowedLifetime = 8 * time.Hour
MaxAllowedLifetime = 8 * time.Hour
AllowedClockSkew = 5 * time.Minute
)

// Key represents a PGP key. It can be a public key or a private & public key pair.
Expand Down Expand Up @@ -100,16 +102,26 @@ func (p *Key) ArmorPublic() (string, error) {
return p.key.GetArmoredPublicKey()
}

// IsExpired returns true if the key is expired with clock skew.
func (p *Key) IsExpired(clockSkew time.Duration) bool {
now := time.Now()

i := p.key.GetEntity().PrimaryIdentity()

expired := func(t time.Time) bool {
return p.key.GetEntity().PrimaryKey.KeyExpired(i.SelfSignature, t) || // primary key has expired
i.SelfSignature.SigExpired(t) // user ID self-signature has expired
}

return expired(now.Add(clockSkew)) && expired(now.Add(-clockSkew))
}

// Validate validates the key.
func (p *Key) Validate() error {
if p.key.IsRevoked() {
return fmt.Errorf("key is revoked")
}

if p.key.IsExpired() {
return fmt.Errorf("key is expired")
}

entity := p.key.GetEntity()
if entity == nil {
return fmt.Errorf("key does not contain an entity")
Expand All @@ -120,6 +132,10 @@ func (p *Key) Validate() error {
return fmt.Errorf("key does not contain a primary identity")
}

if p.IsExpired(AllowedClockSkew) {
return fmt.Errorf("key expired")
}

_, err := mail.ParseAddress(identity.Name)
if err != nil {
return fmt.Errorf("key does not contain a valid email address: %w: %s", err, identity.Name)
Expand All @@ -137,7 +153,7 @@ func (p *Key) validateLifetime() error {
return fmt.Errorf("key does not contain a valid key lifetime")
}

expiration := time.Now().Add(maxAllowedLifetime)
expiration := time.Now().Add(MaxAllowedLifetime)

if !entity.PrimaryKey.KeyExpired(sig, expiration) {
return fmt.Errorf("key lifetime is too long: %s", time.Duration(*sig.KeyLifetimeSecs)*time.Second)
Expand All @@ -158,10 +174,5 @@ func generateEntity(name, comment, email string, lifetimeSecs uint32) (*openpgp.
SigLifetimeSecs: lifetimeSecs,
}

newEntity, err := openpgp.NewEntity(name, comment, email, cfg)
if err != nil {
return nil, err
}

return newEntity, nil
return openpgp.NewEntity(name, comment, email, cfg)
}
83 changes: 83 additions & 0 deletions pkg/pgp/key_test.go
Expand Up @@ -5,9 +5,13 @@
package pgp_test

import (
"crypto"
"testing"
"time"

"github.com/ProtonMail/go-crypto/openpgp"
"github.com/ProtonMail/go-crypto/openpgp/packet"
pgpcrypto "github.com/ProtonMail/gopenpgp/v2/crypto"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand All @@ -30,3 +34,82 @@ func TestKeyFlow(t *testing.T) {
assert.Error(t, key.Verify(message[:len(message)-1], signature))
assert.Error(t, key.Verify(message, signature[:len(signature)-1]))
}

func genKey(t *testing.T, lifetimeSecs uint32, now func() time.Time) *pgp.Key {
cfg := &packet.Config{
Algorithm: packet.PubKeyAlgoEdDSA,
DefaultHash: crypto.SHA256,
DefaultCipher: packet.CipherAES256,
DefaultCompressionAlgo: packet.CompressionZLIB,
KeyLifetimeSecs: lifetimeSecs,
SigLifetimeSecs: lifetimeSecs,
Time: now,
}

entity, err := openpgp.NewEntity("test", "test", "keytest@example.com", cfg)
require.NoError(t, err)

key, err := pgpcrypto.NewKeyFromEntity(entity)
require.NoError(t, err)

pgpKey, err := pgp.NewKey(key)
require.NoError(t, err)

return pgpKey
}

func TestKeyExpiration(t *testing.T) {
for _, tt := range []struct { //nolint:govet
name string
lifetime time.Duration
shift time.Duration
expectedError string
}{
{
name: "no expiration",
expectedError: "key does not contain a valid key lifetime",
},
{
name: "expiration too long",
lifetime: pgp.MaxAllowedLifetime + 1*time.Hour,
expectedError: "key lifetime is too long: 9h0m0s",
},
{
name: "generated in the future",
lifetime: pgp.MaxAllowedLifetime / 2,
shift: pgp.AllowedClockSkew * 2,
expectedError: "key expired",
},
{
name: "already expired",
lifetime: pgp.MaxAllowedLifetime / 2,
shift: -pgp.AllowedClockSkew*2 - pgp.MaxAllowedLifetime/2,
expectedError: "key expired",
},
{
name: "within clock skew -",
lifetime: pgp.MaxAllowedLifetime / 2,
shift: -pgp.AllowedClockSkew / 2,
},
{
name: "within clock skew +",
lifetime: pgp.MaxAllowedLifetime / 2,
shift: pgp.AllowedClockSkew / 2,
},
} {
t.Run(tt.name, func(t *testing.T) {
key := genKey(t, uint32(tt.lifetime/time.Second), func() time.Time {
return time.Now().Add(tt.shift)
})

err := key.Validate()

if tt.expectedError != "" {
assert.Error(t, err)
assert.EqualError(t, err, tt.expectedError)
} else {
assert.NoError(t, err)
}
})
}
}

0 comments on commit cdb9722

Please sign in to comment.