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

getSignedUrl requires private_key #211

Closed
stephenplusplus opened this issue Sep 15, 2014 · 12 comments
Closed

getSignedUrl requires private_key #211

stephenplusplus opened this issue Sep 15, 2014 · 12 comments
Assignees
Labels
api: storage Issues related to the Cloud Storage API. 🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@stephenplusplus
Copy link
Contributor

To generate a signed url, we need a private_key: https://developers.google.com/storage/docs/accesscontrol#Signed-URLs & https://github.com/GoogleCloudPlatform/gcloud-node/blob/v0.6.0/lib/storage/index.js#L267

We get this if a user provides a credentials object or path to a keyfile, but not in GCE or in the future, GAE. Is it going to be possible to get the private key in those environments?

@jgeewax jgeewax changed the title (storage) getSignedUrl requires private_key storage: getSignedUrl requires private_key Oct 5, 2014
@stephenplusplus stephenplusplus changed the title storage: getSignedUrl requires private_key getSignedUrl requires private_key Jan 20, 2015
@stephenplusplus stephenplusplus added the api: storage Issues related to the Cloud Storage API. label Jan 20, 2015
@jgeewax jgeewax added this to the Storage Future milestone Feb 2, 2015
@stephenplusplus
Copy link
Contributor Author

@stephenplusplus
Copy link
Contributor Author

The best solution is to just require private_key as an argument to the function.

@stephenplusplus stephenplusplus self-assigned this Aug 24, 2015
@jgeewax
Copy link
Contributor

jgeewax commented Aug 24, 2015

Hmm... When I get started I have a credentials.json file -- which has a private key as one of the properties... Could we maybe also accept a credentials file?

@stephenplusplus
Copy link
Contributor Author

If they have a keyfile, this will work everywhere:

var gcs = gcloud.storage({ keyFilename: '...keyfile.json' });
gcs.bucket('bucket').file('file').getSignedUrl();

If they don't provide a keyfile, the callback would have received an error. So, I was thinking:

var gcs = gcloud.storage();
gcs.bucket('bucket').file('file').getSignedUrl({
  // ...
  client_email: '...',
  private_key: '...'
}, function() {});

This is similar to how ruby does it: googleapis/google-cloud-ruby#196

And this is already quite an edge case, because we're saying "hey, since you didn't provide a credentials object earlier, how about you give us one now? Are you suuuure you don't have one?"

Any solution really works, as long as we have some way to make it work. Vote?

@jgeewax

  • config.private_key & config.client_email (string)
  • config.credentials (object)
  • config.keyFilename (string)
  • None! Use docs to say "This method only works with a manually authorized instance of Storage. If you haven't already, please provide a keyFilename or credentials object to Storage."

@stephenplusplus

  • config.private_key & config.client_email (string)
  • config.credentials (object)
  • config.keyFilename (string)
  • None! Use docs to say "This method only works with a manually authorized instance of Storage. If you haven't already, please provide a keyFilename or credentials object to Storage."

@callmehiphop

  • config.private_key & config.client_email (string)
  • config.credentials (object)
  • config.keyFilename (string)
  • None! Use docs to say "This method only works with a manually authorized instance of Storage. If you haven't already, please provide a keyFilename or credentials object to Storage."

@jgeewax
Copy link
Contributor

jgeewax commented Aug 24, 2015

Shouldn't we then offer the same argument handling that gcloud.storage() accepts?

var args = <something>
var gcs = gcloud.storage(args);
gcs.bucket('bucket').file('file').getSignedUrl();
gcs.bucket('bucket').file('file').getSignedUrl(args, function() {});

So any valid value of args for gcloud.storage() calls should also work for .getSignedUrl() calls ...?

@stephenplusplus
Copy link
Contributor Author

I think that's weird, since getSignedUrl is a method on a File instance, and just like all of the other methods, it requires a correctly initialized Storage instance to work.

Storage(auth) -> Bucket -> File -> getSignedUrl

It is just weird to one-off our hierarchy and auth:

Storage -> Bucket -> File -> getSignedUrl(auth)

I think this maybe changed my mind, though. If a user ever has a credentials object to give, or a keyfile path, we should say "Give it to .storage()". That way, all of the methods will work as expected. For these two methods, we can add a docs note: "Be sure you have provided a keyfile or credentials object, otherwise these methods will not work."

@jgeewax
Copy link
Contributor

jgeewax commented Aug 24, 2015

I'm just thinking about what's easiest for me. In most cases, storage already has a credentials file, so I'm good. But what if I want to sign as a separate service account?

I'd rather not create a new instance of storage just for that, but I could....

Or I could just pass in whatever would have been valid for storage, but specifically to this specific method -- which is super nice. If I know how to instantiate storage, I know how to sign a URL with the right credentials.


If I had to vote, I'd say either we accept anything that storage accepts, or nothing at all (raising an Exception saying "yo, you can't sign URLs, cuz storage ain't got no credentials on it... set that up first... kthxbai.")

@callmehiphop
Copy link
Contributor

If this is an edge case I think we should just mandate that they create a separate storage instance with the alternative credentials.

@stephenplusplus
Copy link
Contributor Author

I'm just thinking about what's easiest for me.
I'd rather not create a new instance of storage just for that, but I could....

I get you, and I think we can solve "easy" with intuition. For me, that would be, "Oh, this needs auth to work? I'll go back and give it a keyfile." I wouldn't expect each method to support credentials.

raising an Exception saying "yo, you can't sign URLs, cuz storage ain't got no credentials on it... set that up first... kthxbai."

I like this too, but it has to be on the docs level, only because it's an async process to determine if we have the credentials. We can technically see if the properties keyFilename or credentials were given, but we can't know if they're any good or not unless we go through google-auth-library, which means callbacks (async).

The callback will be invoked with some auth error from google-auth-library that should inspire them to check out the docs. We can return a custom error as well, but I'm not sure which is better there (what if the original error was actually helpful to a user who did provide the right credentials, but something about them is off)

@jgeewax
Copy link
Contributor

jgeewax commented Aug 24, 2015

The callback couldn't contain an error saying "we couldn't find any credentials.. you gotta do that!" or "we had credentials, but we tried and they were no good..." ?

I'm all for the callback coming with an err parameter.

@stephenplusplus
Copy link
Contributor Author

Maybe we could wrap it?

getSignedUrl({}, function(err) {
  console.log(err);
  // Signing failed. See `error` property for more.
  // Make sure you gave the correct credentials to your Storage instance.
  //  https://googlecloudplatform.github.io/gcloud-node/#/docs/v0.20.0/storage/file?method=getSignedUrl

  console.log(err.err);
  // Original message from google-auth-library error.
});

@dhermes
Copy link
Contributor

dhermes commented Sep 21, 2015

Sorry I didn't chime in earlier, I had been trying to nudge some Google folks with googleapis/google-cloud-python#922 to support signing a blob on GCE (it's already supported on GAE).

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
sofisl pushed a commit that referenced this issue Sep 16, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
sofisl pushed a commit that referenced this issue Oct 5, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
sofisl pushed a commit that referenced this issue Oct 5, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
sofisl pushed a commit that referenced this issue Nov 11, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/325cd597-d8fe-40d6-aad1-01bd299fa976/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@ba9918c
sofisl pushed a commit that referenced this issue Nov 11, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/cb5a7bf7-f080-4698-bd24-ff5880d64fc8/targets

- [ ] To automatically regenerate this PR, check this box.

PiperOrigin-RevId: 361273630
Source-Link: googleapis/googleapis@5477122
sofisl pushed a commit that referenced this issue Nov 11, 2022
* feat: support regapic LRO

Use gapic-generator-typescript v2.15.1.

PiperOrigin-RevId: 456946341

Source-Link: googleapis/googleapis@88fd18d

Source-Link: googleapis/googleapis-gen@accfa37
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYWNjZmEzNzFmNjY3NDM5MzEzMzM1YzY0MDQyYjA2M2MxYzUzMTAyZSJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* fix: proper camel case for sequences of capital letters

Use gapic-generator-typescript v2.15.2.

PiperOrigin-RevId: 458552034

Source-Link: googleapis/googleapis@ae65014

Source-Link: googleapis/googleapis-gen@b09ede4
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjA5ZWRlNDM1Y2NlMTEwNDQ2ZDRhYjlmNjJhMDgxYjU3MWQzN2UzZiJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
* feat: Add fields for Pub/Sub triggers

Committer: @gleeper
PiperOrigin-RevId: 368533270

Source-Link: googleapis/googleapis@9a9e296

Source-Link: googleapis/googleapis-gen@3735c39

* 🦉 Updates from OwlBot

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
* chore(main): release 2.2.2

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
🤖 I have created a release *beep* *boop*
---


## [4.1.0](googleapis/nodejs-analytics-admin@v4.0.0...v4.1.0) (2022-06-29)


### Features

* support regapic LRO ([#210](googleapis/nodejs-analytics-admin#210)) ([a783ccd](googleapis/nodejs-analytics-admin@a783ccd))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 11, 2022
…211)

- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 417856712

Source-Link: googleapis/googleapis@285ed91

Source-Link: googleapis/googleapis-gen@7d52805
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiN2Q1MjgwNTc3MjRlMWQ0Mjg4MmE3YmNkNjdiZmEwNjk1MWNiZjFjYSJ9
sofisl pushed a commit that referenced this issue Nov 16, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/da9a0346-3aae-4809-a60a-a33e68ec23d5/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@0c868d4
sofisl pushed a commit that referenced this issue Nov 17, 2022
* Update Sendgrid library to latest version
sofisl pushed a commit that referenced this issue Nov 30, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/5a972b54-1689-4186-80b6-fe1833110259/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@0c868d4
sofisl pushed a commit that referenced this issue Jan 26, 2023
…rvice to aiplatform v1 (#211)

Committer: @dizcology
PiperOrigin-RevId: 402573132
Source-Link: googleapis/googleapis@d706102
Source-Link: googleapis/googleapis-gen@3e9242f
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiM2U5MjQyZjY1YmUzYTA3MjVjMGNlZDU2YWUzNDFiOGIwMmMxODliNyJ9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. 🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

5 participants