Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update signing logic for correct custom token format #23

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+28 −6
Open

Update signing logic for correct custom token format #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0b6481
build: add pointycastle package to dependency
kosukesaigusa Feb 9, 2024
963adf2
feat: fix _ServiceAccountSigner sign method to correctly sign token
kosukesaigusa Feb 9, 2024
45f2109
build: remove crypto package from dependency
kosukesaigusa Feb 9, 2024
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
32 changes: 27 additions & 5 deletions packages/dart_firebase_admin/lib/src/utils/crypto_signer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import 'dart:convert';
import 'dart:typed_data';

import 'package:crypto/crypto.dart';
import 'package:googleapis_auth/googleapis_auth.dart' as auth;
import 'package:http/http.dart' as http;
import 'package:meta/meta.dart';
import 'package:pointycastle/pointycastle.dart';

import '../../dart_firebase_admin.dart';

Expand Down Expand Up @@ -107,11 +107,33 @@ class _ServiceAccountSigner implements CryptoSigner {

@override
Future<Uint8List> sign(Uint8List buffer) async {
final key = utf8.encode(credential.privateKey);
final hmac = Hmac(sha256, key);
final digest = hmac.convert(buffer);
final rsaPrivateKey = _parsePrivateKeyFromPem();
final signer = Signer('SHA-256/RSA')
..init(true, PrivateKeyParameter<RSAPrivateKey>(rsaPrivateKey));
final signature = signer.generateSignature(buffer) as RSASignature;
return signature.bytes;
}

RSAPrivateKey _parsePrivateKeyFromPem() {
final privateKeyString = credential.privateKey
.replaceAll('-----BEGIN PRIVATE KEY-----', '')
.replaceAll('-----END PRIVATE KEY-----', '')
.replaceAll('\n', '');
final privateKeyDER = base64Decode(privateKeyString);

final asn1Parser = ASN1Parser(Uint8List.fromList(privateKeyDER));
final topLevelSequence = asn1Parser.nextObject() as ASN1Sequence;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too many casts and ! in there. Could you remove those? Such as by using is and != null

We'd also need tests for when those values are invalid

Copy link
Author

@kosukesaigusa kosukesaigusa Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@rrousselGit

Thank you! I'd like you to give some advice about this point!

I'm thinking to improve the code like the following to reduce as casting and !:

crypto_signer.dart 
RSAPrivateKey _parsePrivateKeyFromPem() {
  final privateKeyString = credential.privateKey
      .replaceAll('-----BEGIN PRIVATE KEY-----', '')
      .replaceAll('-----END PRIVATE KEY-----', '')
      .replaceAll('\n', '');
  final privateKeyDER = base64Decode(privateKeyString);

  final asn1Parser = ASN1Parser(Uint8List.fromList(privateKeyDER));
  final topLevelSequence = asn1Parser.nextObject();
  if (topLevelSequence is! ASN1Sequence) {
    throw const FormatException('Invalid ASN1 format for private key');
  }
  final privateKeyOctet = topLevelSequence.elements?[2];
  if (privateKeyOctet is! ASN1OctetString) {
    throw const FormatException('Invalid ASN1 format for private key');
  }

  final privateKeyParser = ASN1Parser(privateKeyOctet.valueBytes);
  final privateKeySequence = privateKeyParser.nextObject();
  if (privateKeySequence is! ASN1Sequence) {
    throw const FormatException('Invalid ASN1 format for private key');
  }

  final modulus = privateKeySequence.elements?[1];
  final exponent = privateKeySequence.elements?[3];
  final p = privateKeySequence.elements?[4];
  final q = privateKeySequence.elements?[5];
  if (modulus is! ASN1Integer ||
      exponent is! ASN1Integer ||
      p is! ASN1Integer ||
      q is! ASN1Integer) {
    throw const FormatException('Invalid ASN1 format for private key');
  }

  return RSAPrivateKey(
    modulus.integer!,
    exponent.integer!,
    p.integer,
    q.integer,
  );
}

and tried to write the unit test for success case like the following:

crypto_signer_test.dart 
test('parsePrivateKeyFromPem should parse valid private key', () {
  final credential = Credential.fromServiceAccountParams(
    clientId: 'id',
    privateKey: _fakeRSAKey,
    email: 'email',
  );
  final app = FirebaseAdminApp.initializeApp('project-id', credential);
  final signer = cryptoSignerFromApp(app);
  expect(signer, isA<CryptoSigner>());
  expect(signer.algorithm, 'RS256');

  final buffer = Uint8List.fromList(List.generate(32, (i) => i));

  expect(() async => signer.sign(buffer), returnsNormally);
});

However, when I try to write test to confirm the code throwing the FormatException, I need to give invalid private key string to Credential.fromServiceAccountParams, and it throws Invalid DER encoding error before reaching _ServiceAccountSigner.sign (_ServiceAccountSigner._parsePrivateKeyFromPem) method.

https://github.com/google/googleapis.dart/blob/3e5b68f7a81353f8a18ac75ec46c4e1fff2a9b06/googleapis_auth/lib/src/crypto/pem.dart#L100-L102

I think _ServiceAccountSigner is private, so unit test has to be started with the public cryptoSignerFromApp function, which is expected to given FirebaseAdminApp instance.

I appreciate if you have any ideas about it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

One way to do it is to make _ServiceAccountSigner "public", but have export ... hide ServiceAccountSigner.

Make sure to make ServiceAccountSigner with @internal as you do so.

final privateKeyOctet = topLevelSequence.elements![2] as ASN1OctetString;

final privateKeyParser = ASN1Parser(privateKeyOctet.valueBytes);
final privatekeySequence = privateKeyParser.nextObject() as ASN1Sequence;

final modulus = (privatekeySequence.elements![1] as ASN1Integer).integer!;
final exponent = (privatekeySequence.elements![3] as ASN1Integer).integer!;
final p = (privatekeySequence.elements![4] as ASN1Integer).integer;
final q = (privatekeySequence.elements![5] as ASN1Integer).integer;

return Uint8List.fromList(digest.bytes);
return RSAPrivateKey(modulus, exponent, p, q);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/dart_firebase_admin/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ environment:

dependencies:
collection: ^1.18.0
crypto: ^3.0.3
dart_jsonwebtoken: ^2.11.0
firebaseapis: ^0.2.0
freezed_annotation: ^2.4.1
googleapis_auth: ^1.3.0
http: ^1.0.0
intl: ^0.19.0
meta: ^1.9.1
pointycastle: ^3.7.4

dev_dependencies:
build_runner: ^2.4.7
Expand Down