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

feat: add sign method to JWT #375

Merged
merged 4 commits into from
Jun 5, 2018
Merged

feat: add sign method to JWT #375

merged 4 commits into from
Jun 5, 2018

Conversation

JustinBeckwith
Copy link
Contributor

The idea is to replicate what's available here. It's used in nodejs-storage, and is blocking the integration of the new nodejs-common into nodejs-storage.

@stephenplusplus I will admit that I don't entirely know what this is trying to do. Can you walk us through the scenarios in how this is used?

  • Are you sure it belongs in the auth library, or does this belong in nodejs-common?
  • Is this useful on anything other than a service account credential? For example, does it make sense in the context of a Compute credential, or straight up OAuth2?

After we figure out where to stick this thing, I'll write tests 😆

FYI @alexander-fenster @callmehiphop

@JustinBeckwith JustinBeckwith added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 2, 2018
@JustinBeckwith JustinBeckwith requested a review from a team June 2, 2018 03:16
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 2, 2018
@@ -16,7 +16,7 @@

import {GoogleToken} from 'gtoken';
import * as stream from 'stream';

import * as crypto from 'crypto';

This comment was marked as spam.

This comment was marked as spam.

@JustinBeckwith JustinBeckwith mentioned this pull request Jun 2, 2018
@stephenplusplus
Copy link
Contributor

stephenplusplus commented Jun 4, 2018

Ah, this is the grand ole SignBlob API. This was a heavily-requested feature that originated from the Storage API. It's used for getSignedPolicy and getSignedUrl, when the user doesn't provide a credentials object or key file. The private_key property within those are required to "sign" a blob of data locally. If you can't dig one up from what the user provided, you have to hit the API. And in order to do that, you need a client_email.

Are you sure it belongs in the auth library, or does this belong in nodejs-common?

While we're using this exclusively in Storage, I don't think it's only a Storage thing. If we follow the hierarchy of the API endpoint, I guess it's an "IAM" thing. That could go in common, but I think it's closer to an auth thing.

Is this useful on anything other than a service account credential? For example, does it make sense in the context of a Compute credential, or straight up OAuth2?

I'm not exactly sure about the different types. If you can find a private_key, then you can sign locally without making an API request. If not, but you can find a client_email, you can sign using the SignBlob API. If you don't have either, you can't sign anything!

@codecov-io
Copy link

codecov-io commented Jun 4, 2018

Codecov Report

Merging #375 into master will decrease coverage by 0.01%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   94.76%   94.75%   -0.02%     
==========================================
  Files          14       14              
  Lines         956      972      +16     
  Branches      198      201       +3     
==========================================
+ Hits          906      921      +15     
- Misses         50       51       +1
Impacted Files Coverage Δ
src/auth/jwtclient.ts 94.05% <ø> (ø) ⬆️
src/auth/googleauth.ts 94.64% <88.23%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1477db8...8f01353. Read the comment docs.

@JustinBeckwith
Copy link
Contributor Author

This all makes a lot more sense. Given that context, it can be used from the Compute client, or a JWT client, or whatever. I decided to just hang it off GoogleAuth, similar to what you did in google-auto-auth.

Thoughts?

@stephenplusplus
Copy link
Contributor

LGTM!

@@ -16,7 +16,6 @@

import {GoogleToken} from 'gtoken';
import stream from 'stream';

This comment was marked as spam.

@JustinBeckwith JustinBeckwith removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 5, 2018
@JustinBeckwith JustinBeckwith requested review from ofrobots and a team June 5, 2018 15:51
@JustinBeckwith
Copy link
Contributor Author

@alexander-fenster @ofrobots this is ready for a good look over :)

@alexander-fenster
Copy link
Contributor

[sorry, I can only talk about browser stuff now]

I'm fine if you merge it, I'll just move the crypto stuff away to crypto/node/crypto.ts (and will try to implement a browser counterpart) - I'm just not sure about the format of the key that is stored in the client. If SubtleCrypto can't importKey it I guess it's not a big deal because we can always call API to sign, is that right?

@JustinBeckwith
Copy link
Contributor Author

@alexander-fenster this actually shouldn't matter! Since this change only works for Compute and JWT clients, it should have no bearing on a browser implementation. You should probably be stripping out everything but OAuth2Client in your webpack bundle come to think of it.

@JustinBeckwith JustinBeckwith merged commit 2472be0 into googleapis:master Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants