Separate EC key parsing, update related documentation references (#76)

The aim of this is to lay the groundwork for keys other than ES256/P-256. This will make it easier to expand support into other key types (notably RS256)
Firehed · Mar 9, 2024 · 7584b95 · 7584b95
1 parent d77e73e
commit 7584b95
Show file tree

Hide file tree

Showing 9 changed files with 112 additions and 59 deletions.
diff --git a/composer.json b/composer.json
@@ -40,7 +40,7 @@
         "phpstan/phpstan": "^1.0",
         "phpstan/phpstan-phpunit": "^1.0",
         "phpstan/phpstan-strict-rules": "^1.0",
-        "phpunit/phpunit": "^9.3",
+        "phpunit/phpunit": "^9.6",
         "squizlabs/php_codesniffer": "^3.5"
     },
     "scripts": {

diff --git a/src/Attestations/FidoU2F.php b/src/Attestations/FidoU2F.php
@@ -8,6 +8,7 @@
 use Firehed\WebAuthn\AuthenticatorData;
 use Firehed\WebAuthn\BinaryString;
 use Firehed\WebAuthn\Certificate;
+use Firehed\WebAuthn\COSE\Curve;
 use Firehed\WebAuthn\PublicKey\EllipticCurve;
 
 /**
@@ -46,10 +47,7 @@ public function verify(AuthenticatorData $data, BinaryString $clientDataHash): V
         if ($info['type'] !== OPENSSL_KEYTYPE_EC) {
             throw new \Exception('Certificate PubKey is not Elliptic Curve');
         }
-        // OID for P-156 curve
-        // http://oid-info.com/get/1.2.840.10045.3.1.7
-        // See also EllipticCurve
-        if ($info['ec']['curve_oid'] !== '1.2.840.10045.3.1.7') {
+        if ($info['ec']['curve_oid'] !== Curve::P256->getOid()) {
             throw new \Exception('Certificate PubKey is not Elliptic Curve');
         }
 

diff --git a/src/AuthenticatorData.php b/src/AuthenticatorData.php
@@ -11,6 +11,7 @@
  * @internal
  *
  * @link https://www.w3.org/TR/webauthn-3/#sctn-authenticator-data
+ * @see §6.5
  */
 class AuthenticatorData
 {

diff --git a/src/COSE/Algorithm.php b/src/COSE/Algorithm.php
@@ -5,8 +5,10 @@
 namespace Firehed\WebAuthn\COSE;
 
 /**
- * @link https://www.rfc-editor.org/rfc/rfc8152.html
- * @see Section 8.1, table 5
+ * @link https://www.rfc-editor.org/rfc/rfc9053.html
+ * @see §2.1, table 1
+ *
+ * @link https://www.iana.org/assignments/cose/cose.xhtml#algorithms
  */
 enum Algorithm: int
 {

diff --git a/src/COSE/Curve.php b/src/COSE/Curve.php
@@ -5,19 +5,37 @@
 namespace Firehed\WebAuthn\COSE;
 
 /**
- * @link https://www.rfc-editor.org/rfc/rfc8152.html
- * @see Section 13.1, table 22
+ * @link https://www.rfc-editor.org/rfc/rfc9053.html
+ * @see §7.1, table 18
+ *
+ * @link https://www.iana.org/assignments/cose/cose.xhtml#elliptic-curves
  */
 enum Curve: int
 {
+    // OIDs: RFC5840 §2.1.1.1
+
     // secp256r1 = 1.2.840.10045.3.1.7
     case P256 = 1; // EC2
+
     // secp384r1 = 1.3.132.0.34
     case P384 = 2; // EC2
+
     // secp521r1 = 1.3.132.0.35
     case P521 = 3; // EC2 (*not* 512)
+
     case X25519 = 4; // OKP
+
     case X448 = 5; // OKP
+
     case ED25519 = 6; // OKP
+
     case ED448 = 7; // OKP
+
+    public function getOid(): string
+    {
+        return match ($this) { // @phpstan-ignore-line default unhandled match is desired
+            self::P256 => '1.2.840.10045.3.1.7',
+            // TODO: add others as support increases
+        };
+    }
 }
diff --git a/src/COSE/KeyType.php b/src/COSE/KeyType.php
@@ -5,8 +5,10 @@
 namespace Firehed\WebAuthn\COSE;
 
 /**
- * @link https://www.rfc-editor.org/rfc/rfc8152.html
- * @see Section 13, table 21
+ * @link https://www.rfc-editor.org/rfc/rfc9053.html
+ * @see §7, table 17
+ *
+ * @link https://www.iana.org/assignments/cose/cose.xhtml#key-type
  */
 enum KeyType: int
 {

diff --git a/src/COSEKey.php b/src/COSEKey.php
@@ -23,26 +23,23 @@
  * @link https://www.rfc-editor.org/rfc/rfc8152.html
  *
  * @see RFC 8230 (RSA key support - not yet implemented)
+ *
+ * @see RFC 9052
+ * @link https://www.rfc-editor.org/rfc/rfc9052
  */
 class COSEKey
 {
     // Data structure indexes
-    // @see section 7.1
-    private const INDEX_KEY_TYPE = 1;
-    private const INDEX_ALGORITHM = 3;
-    // 13.1.1-13.2
-    private const INDEX_CURVE = -1; // ECC, OKP
-    private const INDEX_X_COORDINATE = -2; // ECC, OKP
-    private const INDEX_Y_COORDINATE = -3; // ECC
-    private const INDEX_PRIVATE_KEY = -4; // ECC, OKP @phpstan-ignore-line
-    // index_key_value = -1 (same as index_curve, for Symmetric)
+    // @see RFC 9052 §7.1
+    public const INDEX_KEY_TYPE = 1;
+    public const INDEX_KEY_ID = 2;
+    public const INDEX_ALGORITHM = 3;
+    public const INDEX_KEY_OPS = 4;
+    public const INDEX_BASE_IV = 5;
 
-    private COSE\KeyType $keyType;
+    private PublicKey\PublicKeyInterface $publicKey;
+    // TODO: move to PublicKeyInterface?
     public readonly COSE\Algorithm $algorithm;
-    private COSE\Curve $curve;
-    private BinaryString $x;
-    private BinaryString $y;
-    // d ~ private key
 
     public function __construct(public readonly BinaryString $cbor)
     {
@@ -55,31 +52,12 @@ public function __construct(public readonly BinaryString $cbor)
             throw new DomainException('Only EC2 keys supported');
         }
 
-        $algorithm = COSE\Algorithm::tryFrom($decodedCbor[self::INDEX_ALGORITHM]);
-        if ($algorithm !== COSE\Algorithm::EcdsaSha256) {
-            throw new DomainException('Only ES256 supported');
-        }
-
-        $curve = COSE\Curve::tryFrom($decodedCbor[self::INDEX_CURVE]);
-        if ($curve !== COSE\Curve::P256) {
-            throw new DomainException('Only curve P-256 (secp256r1) supported');
-        }
-
-        $this->keyType = $keyType;
-        $this->algorithm = $algorithm;
-        $this->curve = $curve;
-
-        if (strlen($decodedCbor[self::INDEX_X_COORDINATE]) !== 32) {
-            throw new DomainException('X coordinate not 32 bytes');
-        }
-        $this->x = new BinaryString($decodedCbor[self::INDEX_X_COORDINATE]);
-
-        if (strlen($decodedCbor[self::INDEX_Y_COORDINATE]) !== 32) {
-            throw new DomainException('X coordinate not 32 bytes');
-        }
-        $this->y = new BinaryString($decodedCbor[self::INDEX_Y_COORDINATE]);
+        $this->publicKey = match ($keyType) {
+            COSE\KeyType::EllipticCurve => PublicKey\EllipticCurve::fromDecodedCbor($decodedCbor),
+        };
 
-        // d = cbor[INDEX_PRIVATE_KEY]
+        assert(array_key_exists(self::INDEX_ALGORITHM, $decodedCbor));
+        $this->algorithm = COSE\Algorithm::from($decodedCbor[self::INDEX_ALGORITHM]);
 
         // Future: rfc8152/13.2
         // if keytype == .OctetKeyPair, set `x` and `d`
@@ -90,12 +68,6 @@ public function __construct(public readonly BinaryString $cbor)
      */
     public function getPublicKey(): PublicKey\PublicKeyInterface
     {
-        // These are valid; the internal formats are brittle right now.
-        assert($this->keyType === COSE\KeyType::EllipticCurve);
-        assert($this->curve === COSE\Curve::P256);
-        // This I don't think conveys anything useful. Mostly retained to
-        // silence a warning about unused variables.
-        assert($this->algorithm === COSE\Algorithm::EcdsaSha256);
-        return new PublicKey\EllipticCurve($this->x, $this->y);
+        return $this->publicKey;
     }
 }
diff --git a/src/PublicKey/EllipticCurve.php b/src/PublicKey/EllipticCurve.php
@@ -4,7 +4,10 @@
 
 namespace Firehed\WebAuthn\PublicKey;
 
+use DomainException;
 use Firehed\WebAuthn\BinaryString;
+use Firehed\WebAuthn\COSE;
+use Firehed\WebAuthn\COSEKey;
 use UnexpectedValueException;
 
 /**
@@ -20,8 +23,19 @@
  */
 class EllipticCurve implements PublicKeyInterface
 {
-    public function __construct(private BinaryString $x, private BinaryString $y)
-    {
+    // CBOR decoding: RFC 9053 §7.1.1
+    private const INDEX_CURVE = -1; // ECC, OKP
+    private const INDEX_X_COORDINATE = -2; // ECC, OKP
+    private const INDEX_Y_COORDINATE = -3; // ECC
+    private const INDEX_PRIVATE_KEY = -4; // ECC, OKP @phpstan-ignore-line
+    // index_key_value = -1 (same as index_curve, for Symmetric)
+
+
+    public function __construct(
+        private COSE\Curve $curve,
+        private BinaryString $x,
+        private BinaryString $y,
+    ) {
         if ($x->getLength() !== 32) {
             throw new UnexpectedValueException('X-coordinate not 32 bytes');
         }
@@ -30,6 +44,47 @@ public function __construct(private BinaryString $x, private BinaryString $y)
         }
     }
 
+    /**
+     * @param mixed[] $decoded
+     */
+    public static function fromDecodedCbor(array $decoded): EllipticCurve
+    {
+        // Checked upstream, but re-verify
+        assert(array_key_exists(COSEKey::INDEX_KEY_TYPE, $decoded));
+        $type = COSE\KeyType::from($decoded[COSEKey::INDEX_KEY_TYPE]);
+        assert($type === COSE\KeyType::EllipticCurve);
+
+
+        assert(array_key_exists(COSEKey::INDEX_ALGORITHM, $decoded));
+        $algorithm = COSE\Algorithm::from($decoded[COSEKey::INDEX_ALGORITHM]);
+        // TODO: support other algorithms
+        if ($algorithm !== COSE\Algorithm::EcdsaSha256) {
+            throw new DomainException('Only ES256 is supported');
+        }
+
+        $curve = COSE\Curve::from($decoded[self::INDEX_CURVE]);
+        // WebAuthn §5.8.5 - cross-reference curve to algorithm
+        assert($curve === COSE\Curve::P256);
+
+        if (strlen($decoded[self::INDEX_X_COORDINATE]) !== 32) {
+            throw new DomainException('X coordinate not 32 bytes');
+        }
+        $x = new BinaryString($decoded[self::INDEX_X_COORDINATE]);
+
+        if (strlen($decoded[self::INDEX_Y_COORDINATE]) !== 32) {
+            throw new DomainException('X coordinate not 32 bytes');
+        }
+        $y = new BinaryString($decoded[self::INDEX_Y_COORDINATE]);
+
+        // private key should not be present; ignoring it
+
+        return new EllipticCurve(
+            curve: $curve,
+            x: $x,
+            y: $y,
+        );
+    }
+
     /**
      * Returns a 32-byte string representing the 256-bit X-coordinate on the
      * curve
@@ -52,7 +107,11 @@ public function getYCoordinate(): BinaryString
     // public key component
     public function getPemFormatted(): string
     {
+        if ($this->curve !== COSE\Curve::P256) {
+            throw new DomainException('Only P256 curves can be PEM-formatted so far');
+        }
         // Described in RFC 5480
+        // §2.1.1.1
         // Just use an OID calculator to figure out *that* encoding
         $der = hex2bin(
             '3059' // SEQUENCE, length 89

diff --git a/tests/PublicKey/EllipticCurveTest.php b/tests/PublicKey/EllipticCurveTest.php
@@ -5,6 +5,7 @@
 namespace Firehed\WebAuthn\PublicKey;
 
 use Firehed\WebAuthn\BinaryString;
+use Firehed\WebAuthn\COSE\Curve;
 
 /**
  * @covers Firehed\WebAuthn\PublicKey\EllipticCurve
@@ -30,7 +31,7 @@ public function testFormatHandling(): void
         $x = BinaryString::fromHex('0f06777d44842cce4a2e7d00587b3fc892a7da7cf1704a8dd1ffb7e5334721a8');
         $y = BinaryString::fromHex('3f017188437532409d6bbc86b68d56214a720bf8c183f844c576f4e2003ba976');
 
-        $pk = new EllipticCurve($x, $y);
+        $pk = new EllipticCurve(Curve::P256, $x, $y);
         self::assertTrue($x->equals($pk->getXCoordinate()), 'X-coordinate changed');
         self::assertTrue($y->equals($pk->getYCoordinate()), 'Y-coordinate changed');