From 6843408d42e477ce356632e05968ccb062d544e8 Mon Sep 17 00:00:00 2001
From: Ivan Bertona <ivan@ibrt.me>
Date: Fri, 7 Feb 2020 19:26:18 -0500
Subject: [PATCH] Reject obsolete id-pe-acmeIdentifier.

---
 acme/challenge.go      |  17 ++++++-
 acme/challenge_test.go | 100 +++++++++++++++++++----------------------
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index d55f42a6..986b94c4 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -439,6 +439,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val
 
 	idPeAcmeIdentifier := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 31}
 	idPeAcmeIdentifierV1Obsolete := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1}
+	foundIDPeAcmeIdentifierV1Obsolete := false
 
 	keyAuth, err := KeyAuthorization(tc.Token, jwk)
 	if err != nil {
@@ -447,8 +448,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val
 	hashedKeyAuth := sha256.Sum256([]byte(keyAuth))
 
 	for _, ext := range leafCert.Extensions {
-		if idPeAcmeIdentifier.Equal(ext.Id) || idPeAcmeIdentifierV1Obsolete.Equal(ext.Id) {
-
+		if idPeAcmeIdentifier.Equal(ext.Id) {
 			if !ext.Critical {
 				if err = tc.storeError(db,
 					RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+
@@ -490,6 +490,19 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val
 			}
 			return upd, nil
 		}
+
+		if idPeAcmeIdentifierV1Obsolete.Equal(ext.Id) {
+			foundIDPeAcmeIdentifierV1Obsolete = true
+		}
+	}
+
+	if foundIDPeAcmeIdentifierV1Obsolete {
+		if err = tc.storeError(db,
+			RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+
+				"obsolete id-pe-acmeIdentifier in acmeValidationV1 extension"))); err != nil {
+			return nil, err
+		}
+		return tc, nil
 	}
 
 	if err = tc.storeError(db,
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index 40466454..4d97d79d 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -1463,6 +1463,52 @@ func TestTLSALPN01Validate(t *testing.T) {
 				res: ch,
 			}
 		},
+		"ok/obsolete-oid": func(t *testing.T) test {
+			ch, err := newTLSALPNCh()
+			assert.FatalError(t, err)
+			oldb, err := json.Marshal(ch)
+			assert.FatalError(t, err)
+
+			jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0)
+			assert.FatalError(t, err)
+
+			expErr := RejectedIdentifierErr(errors.New("incorrect certificate for tls-alpn-01 challenge: " +
+				"obsolete id-pe-acmeIdentifier in acmeValidationV1 extension"))
+			baseClone := ch.clone()
+			baseClone.Error = expErr.ToACME()
+			newCh := &tlsALPN01Challenge{baseClone}
+			newb, err := json.Marshal(newCh)
+			assert.FatalError(t, err)
+
+			expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk)
+			assert.FatalError(t, err)
+			expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth))
+
+			cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], true, true, ch.getValue())
+			assert.FatalError(t, err)
+
+			srv, tlsDial := newTestTLSALPNServer(cert)
+			srv.Start()
+
+			return test{
+				srv: srv,
+				ch:  ch,
+				vo: validateOptions{
+					tlsDial: tlsDial,
+				},
+				jwk: jwk,
+				db: &db.MockNoSQLDB{
+					MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) {
+						assert.Equals(t, bucket, challengeTable)
+						assert.Equals(t, key, []byte(ch.getID()))
+						assert.Equals(t, old, oldb)
+						assert.Equals(t, string(newval), string(newb))
+						return nil, true, nil
+					},
+				},
+				res: ch,
+			}
+		},
 		"ok/with-new-oid": func(t *testing.T) test {
 			ch, err := newTLSALPNCh()
 			assert.FatalError(t, err)
@@ -1525,60 +1571,6 @@ func TestTLSALPN01Validate(t *testing.T) {
 				res: newCh,
 			}
 		},
-		"ok/with-obsolete-oid": func(t *testing.T) test {
-			ch, err := newTLSALPNCh()
-			assert.FatalError(t, err)
-			_ch, ok := ch.(*tlsALPN01Challenge)
-			assert.Fatal(t, ok)
-			_ch.baseChallenge.Error = MalformedErr(nil).ToACME()
-			oldb, err := json.Marshal(ch)
-			assert.FatalError(t, err)
-
-			baseClone := ch.clone()
-			baseClone.Status = StatusValid
-			baseClone.Error = nil
-			newCh := &tlsALPN01Challenge{baseClone}
-
-			jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0)
-			assert.FatalError(t, err)
-
-			expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk)
-			assert.FatalError(t, err)
-			expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth))
-
-			cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], true, true, ch.getValue())
-			assert.FatalError(t, err)
-
-			srv, tlsDial := newTestTLSALPNServer(cert)
-			srv.Start()
-
-			return test{
-				srv: srv,
-				ch:  ch,
-				vo: validateOptions{
-					tlsDial: tlsDial,
-				},
-				jwk: jwk,
-				db: &db.MockNoSQLDB{
-					MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) {
-						assert.Equals(t, bucket, challengeTable)
-						assert.Equals(t, key, []byte(ch.getID()))
-						assert.Equals(t, old, oldb)
-
-						alpnCh, err := unmarshalChallenge(newval)
-						assert.FatalError(t, err)
-						assert.Equals(t, alpnCh.getStatus(), StatusValid)
-						assert.True(t, alpnCh.getValidated().Before(time.Now().UTC().Add(time.Minute)))
-						assert.True(t, alpnCh.getValidated().After(time.Now().UTC().Add(-1*time.Second)))
-
-						baseClone.Validated = alpnCh.getValidated()
-
-						return nil, true, nil
-					},
-				},
-				res: newCh,
-			}
-		},
 	}
 	for name, run := range tests {
 		t.Run(name, func(t *testing.T) {