firebase / firebase-admin-node

Firebase Admin Node.js SDK
https://firebase.google.com/docs/admin/setup
Apache License 2.0
1.6k stars 358 forks source link

Application Credentials are not fully parsed/validated during initializeApp #1062

Open jmealo opened 3 years ago

jmealo commented 3 years ago

[READ] Step 1: Are you in the right place?

Yes

[REQUIRED] Step 2: Describe your environment

[REQUIRED] Step 3: Describe the problem

Steps to reproduce:

  1. Copy JSON-encoded Google Application Credentials from a browser based tool like Bitwarden
  2. Attempt to use the SDK with those credentials (valid JSON containing a poorly-encoded/formatted PEM)
  3. Initialize the app without error
  4. Send an FCM message, receive an error: error: { "library": "PEM routines", "function": "get_name", "reason": "no start line", "code": "ERR_OSSL_PEM_NO_START_LINE" }

What I expect to happen: I would like the error to be thrown when I initializeApp during the bootstrapping of our application so I can treat it as a configuration error, rather than an error we hit at runtime the first time that a message is sent.

Relevant Code:

  const FCM_TOKEN = JSON.parse(GOOGLE_APPLICATION_CREDENTIALS)

  const serviceAccount = {
    projectId: FCM_TOKEN.project_id,
    clientEmail: FCM_TOKEN.client_email,
    privateKey: FCM_TOKEN.private_key
  }

  // Shouldn't this to throw if the credentials cannot be parsed?
  firebaseAdmin.initializeApp({
    credential: firebaseAdmin.credential.cert(serviceAccount),
    databaseURL: 'https://xxxx.firebaseio.com'
  })
google-oss-bot commented 3 years ago

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

hiranya911 commented 3 years ago

credential.cert() already checks for a variety of user errors and throws eagerly:

https://github.com/firebase/firebase-admin-node/blob/738250da37df9261ab0b76a39cf8c6d223cb766e/test/unit/credential/credential.spec.ts#L102-L162

The private key is parsed using the node-forge library to check for common errors. What kind of error was in your private key that couldn't be detected?

jmealo commented 3 years ago

There was no visible defect. It's hard to read JSON embedded in YAML. Possibly non-printable characters or issues with newlines if I had to guess from the Open SSL error. We had developers on OSX and Linux hit this issue. It occurs when copying credentials from a browser based store into a YAML file. This is a pretty typical use case these days, so, I decided to report it.

When calling firebase.messaging().sendMulticast(messageData) we get an error: { "library": "PEM routines", "function": "get_name", "reason": "no start line", "code": "ERR_OSSL_PEM_NO_START_LINE" }

Perhaps there's an upstream issue with node-forge or it's configured to be less strict than Open SSL. Given that node-forge doesn't appear to catch all errors, I would recommend try { } catch around the actual parsing function to prevent the reported issue.

hiranya911 commented 3 years ago

We already perform all the parsing in the admin.credential.cert() function:

https://github.com/firebase/firebase-admin-node/blob/738250da37df9261ab0b76a39cf8c6d223cb766e/src/credential/credential-internal.ts#L152-L187

If you're not getting any errors from that method, then the error has been encountered during JWT signing:

https://github.com/firebase/firebase-admin-node/blob/738250da37df9261ab0b76a39cf8c6d223cb766e/src/credential/credential-internal.ts#L119-L128

There's no clean way to force this to occur eagerly. And I don't see how adding a try-catch block around this is going to make the error show up during SDK initialization. We probably need to perform some additional check during initial parsing for that. But without knowing what exact issue in the input is causing this, it's not possible to reason about what that check should be.

Follow up question: Suppose the SDK does throw an error at initialization in this case. What steps can you take to rectify it? How can you fix the credentials/private key to mitigate the error?

jmealo commented 3 years ago

RE: Follow-up question: Restore the credentials from a known-good original source. Knowing that you have to do that is the tricky part though. The error is detached from the cause for the developer.

It doesn't look like sign does any verification on the key. It calls into createSigner (from node-jws)... this creates a streaming interface for signing. It doesn't look like auth0 is actively supporting these modules anymore.

hiranya911 commented 3 years ago

Restore the credentials from a known-good original source. Knowing that you have to do that is the tricky part though. The error is detached from the cause for the developer.

I'd recommend investing some time into figuring out how the credential became malformed in the first place, and taking some measures to prevent that. Otherwise it's just going to happen repeatedly. Also if we know the exact way the credential has become invalid, we can check for that during SDK initialization.

It doesn't look like auth0 is actively supporting these modules anymore.

It is still the most widely used jwt library for Node.js to date (with 4.8 million downloads weekly). The reason why it hasn't seen many changes lately is probably because much of it is quite stable.

One possible way to handle this is to call jwt.sign() at the SDK initialization (instead of node-forge) and use that as the early error checking mechanism. The sign() is a synchronous API, which permits this. But without a repro of the error we don't have a way to test such a change.

jmealo commented 3 years ago

I think that your proposed solution makes sense. Would you accept a PR with a test case?

hiranya911 commented 3 years ago

I think that your proposed solution makes sense. Would you accept a PR with a test case?

That would be great!