-
Notifications
You must be signed in to change notification settings - Fork 102
Convert SAS times to UTC before formatting #171
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
base: dev
Are you sure you want to change the base?
Conversation
FYI, as-written this will break SAS round-tripping for signatures generated with non-UTC timestamps, just like SAS round-tripping gets broken by dropping timestamp precision as identified in #168. |
Hi @someone1, thanks for the PR! Could you please address the comment from @AtOMiCNebula? |
@zezha-msft - I can certainly try! @AtOMiCNebula - I don't know if I follow as I'm not thoroughly familiar with the process you're describing. Do you mind elaborating a bit? Seeing #169 - I'm inclined to just convert The timestamps seem always to be assumed to be in UTC in the formats - so I'm not sure how you you can generate valid signatures with non-UTC timestamps as you've stated. |
@someone1 This library has a bad habit of rewriting components of SASs, which breaks the ability to actually use the SAS. The fix of mine that I referred to earlier was to address an issue where AzCopy was unable to use a SAS produced by Azure itself, because Azure was handing out SASs with timestamps that included a microseconds component. This SDK stripped out the microseconds, and you can't do that, since the signature is invalidated (by design) if you change any of the parts. My concern is that this change may break round-tripping (though perhaps I came off a bit strong in my earlier statement). The scenario I have in mind is that a string timestamp is parsed into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do the needful :)
ss := "" | ||
if !startTime.IsZero() { | ||
ss = startTime.Format(SASTimeFormat) // "yyyy-MM-ddTHH:mm:ssZ" | ||
ss = startTime.UTC().Format(SASTimeFormat) // "yyyy-MM-ddTHH:mm:ssZ" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @someone1 ,
Thanks for reaching out !
We are happy to see your contribution.
There are couple of things which I'd like to point out
- We always merge to
dev
branch before releasing it to master. So I changed the base todev
branch - There is a conflict in merge which will be removed if you do the required changes in either
formatSASTimeWithDefaultFormat()
function or send UTC time as argument itself.
Also, it'd be nice if you can see whether formatSASTimeWithDefaultFormat()
func is getting used some other function call because if that is the case, we need to see if UTC time is required in that function or not?
Fixes #170