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.

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

Update signing logic for correct custom token format #23

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kosukesaigusa
Copy link

@kosukesaigusa kosukesaigusa commented Feb 9, 2024

Background

Create custom token here:

final adminApp = FirebaseAdminApp.initializeApp(
  'project-id',
  Credential.fromServiceAccountParams(
    clientId: 'client-id',
    privateKey: 'private-key',
    email: 'email',
  ),
);
final auth = Auth(adminApp);
final customToken = await auth.createCustomToken('some-user-id');
print('customToken: $customToken');

Then, try to use the custom token to sign in Firebase Auth with on Flutter client app:

final userCredential = await FirebaseAuth.instance.signInWithCustomToken(customToken);

The exception like the following is thrown:

FirebaseAuthException ([firebase_auth/invalid-custom-token] The custom token format is incorrect. Please check the documentation.)

If I use the custom token created by JS (TS) SDK by the same project service account on my Flutter client app, it successfully signs in.

admin.initializeApp({
    credential: admin.credential.cert(serviceAccount),
    databaseURL: `https://${serviceAccount.projectId}.firebaseio.com`
})

const main = async () => {
    const customToken = await admin.auth().createCustomToken(`some-user-id`)
    console.log(`customToken: ${customToken}`)
}

main()

I noticed the length of custom tokens are different from each other.

# Example of created custom token by this package:
eyJ*****************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************z0A

# Example of created custom token by JS (TS) SDK:
eyJ****************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************uFA

Then this PR fixes the RSA-SHA256 signing logic using pointycastle package instead of crypto package.

After the change,

  • The length of created custom tokens is the same as the ones created by JS (TS) SDK.
  • Successful signing with custom token is confirmed.

I am so excited by and curious about this project, enabling us to write Firebase Admin SDK server-side code by Dart!

Thank you so much for developing such a wonderful project!

@CLAassistant
Copy link

CLAassistant commented Feb 9, 2024

CLA assistant check
All committers have signed the CLA.

@rrousselGit
Copy link
Collaborator

rrousselGit commented Feb 28, 2024

Hello! Sorry for the delay.

We'd need tests for this. Could you add those? Otherwise LGTM

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

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.

@kosukesaigusa
Copy link
Author

@rrousselGit

Thank you for your comment!

I will add tests and update the code based on your feedback!

blaugold added a commit to lotum/dart_firebase_admin that referenced this pull request Mar 19, 2024
@gildaswise
Copy link

Any updates on this? This lib is still not working with createCustomToken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants