-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support private HTTP template URLs #39
Conversation
ba744d1
to
f9919d8
Compare
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.
lgtm
src/cfn/index.ts
Outdated
@@ -546,6 +582,11 @@ async function loadCFNTemplate(location: string, baseLocation: string): | |||
if (_.isUndefined(location)) { | |||
return {}; | |||
} | |||
if (location.match(/^http/) !== null) { |
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.
What happens if the location is already a signed s3 url (possibly from a different account)?
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.
From what I can tell, S3 doesn't support signature v2 which has a maximum one year expiry. Signature v4 has a maximum 7 day expiry. Given that signed URLs have such a short lifetime, I don't think this is an edge case we need to support right now. I think a better solution would be to give the current IAM principal access to the other account's bucket or just use a bucket in the same account.
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.
Regardless of the expiration issue, I think we need to respect that other tooling might be passing one in. A check for Signature=
in the location
would suffice to handle that case.
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.
I think we should also add a comment explaining why we're checking / rewriting the URL here and how this use case is different from the s3://...
url rewrite that this function does on lines 601-608.
Seems good |
It looks like this introduced a bug for rendered templates -
|
Use signed URL for
http
/https
Template
s to support non-public readable templates hosted on S3.