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

Queuesas #657

Merged
merged 14 commits into from
Jul 3, 2017
Merged

Queuesas #657

merged 14 commits into from
Jul 3, 2017

Conversation

kpfaulkner
Copy link
Contributor

Generating SAS URLs for Storage queues. Similar approach to the blob SAS generation.
Related issue #656

One problem I haven't been able to resolve is that if I'm reading the specs ( https://docs.microsoft.com/en-us/rest/api/storageservices/constructing-a-service-sas ) correctly I should be able to specify the protocol to use (http and/or https). Currently I cannot get that working, but leaving it as blank IS accepted by the spec (and works).

@mcardosos
Copy link
Contributor

Hello @kpfaulkner !
Thanks for submitting a PR.
While looking at you PR, I noticed some other issues there are in the already checked-in code, so I addressed them on this other PR. Your PR and mine fix stuff in a somewhat different way, but it would be cool if both PRs were consistent (for example, for the SAS permissions, I liked very much the pattern you used in #422 with the CanRead, CanWrite and CanDelete bools, so I preferred to follow that pattern for SAS implementation too). I have also discussed this with @jhendrixMSFT . Do the changes in #662 make sense to you? Feel free to comment it if something does not feel right :) Meanwhile I will take a look on the issues you have mentioned in your PR's comments and how to fix them (the identifier, start time and protocols).

@kpfaulkner
Copy link
Contributor Author

kpfaulkner commented Jun 22, 2017

Cool, yes am able to restructure this PR to be a similar style to #662 and #422.

Is #662 getting merged soon?

signedExpiry := expiry.UTC().Format(time.RFC3339)

// Cannot get this working yet. Any values entered generates bad URL.
protocols := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this generate a bad URL? I played a little with the protocols option but I did not find problems. Is there something I missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whenever I had a value for "protocols" it wouldn't work. Empty string would be the only value I could get working. You're not seeing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try and repro later today. But whenever I had a value I'd never get it to generate a URL successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example if protocol is populated with "https" I generate the SAS URL but when trying to use it I get the error:

AuthenticationFailed Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature. RequestId:0e3aae9d-0003-002f-3e81-ec31f4000000 Time:2017-06-24T00:31:37.0443316Z Signature did not match. String to sign used was p 2017-06-24T01:31:26Z /queue/kenfau/temp 2016-05-31

But protocol of an empty string is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR to follow similar lines to @mcardosos BlobSAS changes.... although I still cannot get protocol to work properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, so the protocol should be included both in the string to sign and on the query. Curious question, when trying to use the SAS URI, are you sending the request with an authorization header?

Copy link
Contributor

Choose a reason for hiding this comment

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

My string to sign looks like this...

raup

2050-12-20T21:55:06Z
/queue/golangrocksonazure/name


https,http
2016-05-31

And if I do (manually) a Get queue metadata operation, the URL looks like this

https://golangrocksonazure.queue.core.windows.net/name?comp=metadata&se=2050-12-20T21%3A55%3A06Z&sig=nGP%2BOUcNIc5i%2Fcb1sFD2Ksnpz1CHazjdVy2ewurTUsE%3D&sp=raup&spr=https%2Chttp&sv=2016-05-31

When I get the same error as you is when I don't include the protocols in the URL as query parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh ok, my bad, I wasn't including them as a parameter as well.

@mcardosos
Copy link
Contributor

Merged #662 :D

@kpfaulkner
Copy link
Contributor Author

Everything looking good now.

}

// assumption that start time is now.
signedStart := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumption is done by Azure, not the SDK. I would prefer this comment to go away.

return sasURL.String(), nil
}

func queueSASStringToSign(signedVersion, canonicalizedResource, signedStart, signedExpiry, signedIP string, signedPermissions string, protocols string, signedIdentifier string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

string [](start = 100, length = 6)

Nit, "string" is not needed by all parameters here.

@mcardosos
Copy link
Contributor

@kpfaulkner Cool! just some last tiny comments, but other than that, LGTM :D

@mcardosos mcardosos merged commit e4541c8 into Azure:dev Jul 3, 2017
@mcardosos
Copy link
Contributor

Awesome! @kpfaulkner
Thanks for contributing!

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.

3 participants