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

Support authenticated HTTP URLs (fixes #851) #852

Merged
merged 2 commits into from
Apr 16, 2019

Conversation

LeeXGreen
Copy link
Contributor

This PR fixes issue #851.

Description

Software source method now supports the authorization option.
This allows the user to supply an HTTP Authorization header which will
be passed to the source URL.

For the common case of basic authentication, that looks like this:

require 'base64'

username = 'XXX'
password = 'YYY'
credentials = Base64.encode64("#{username}:#{password}")

version 'the_version' do
  source url: 'https://example.com/path/to/the/file',
         authorization: "Basic #{credentials}",
         ...
end

Maintainers

Please ensure that you check for:

  • [] If this change impacts git cache validity, it bumps the git cache
    serial number
  • [] If this change impacts compatibility with omnibus-software, the
    corresponding change is reviewed and there is a release plan
  • [] If this change impacts compatibility with the omnibus cookbook, the
    corresponding change is reviewed and there is a release plan

@LeeXGreen LeeXGreen requested a review from a team as a code owner August 27, 2018 16:15
@LeeXGreen LeeXGreen force-pushed the netfetcher_http_authorization branch from 6524733 to 248d419 Compare August 27, 2018 16:17
@niekrasp
Copy link

niekrasp commented Sep 3, 2018

How can username and password be passed here when it would be build e.g. via CI ?
Jenkins Slave uses kitchen to start vm on which omnibus build will happen.

@LeeXGreen
Copy link
Contributor Author

LeeXGreen commented Sep 3, 2018

@niekrasp We typically pass the credentials in as an environment variable, e.g.

require 'base64'

username = ENV['ARTIFACTORY_USERNAME']
password = ENV['ARTIFACTORY_PASSWORD']
credentials = Base64.encode64("#{username}:#{password}")

version 'the_version' do
  source url: 'https://example.com/path/to/the/file',
         authorization: "Basic #{credentials}",
         ...
end

Then it's just up to Jenkins (or whatever CI system) to set the ENV variable when it launches the VM.

@niekrasp
Copy link

@LeeXGreen thanks !

When this PR can be merged ?

@niekrasp
Copy link

@lamont-granquist as follow up to #856, can you or someone else review&merge this PR as well ? please :)
Thank you

@LeeXGreen
Copy link
Contributor Author

Is there anything I can do to help this get merged?
I'd love to return to using upstream omnibus, instead of my fork :)

@LeeXGreen
Copy link
Contributor Author

Bump -- this seems to be a fairly common case when building omnibus installers in an enterprise environment

@marcparadise
Copy link
Member

@LeeXGreen thanks for submitting this.

This change looks good. Could you get the branch up to date, and add tests to show NetFetcher#download is passing the correct options to download_file?

@marcparadise marcparadise self-assigned this Mar 29, 2019
@marcparadise marcparadise added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Mar 29, 2019
@LeeXGreen LeeXGreen force-pushed the netfetcher_http_authorization branch from a9d5e17 to 8559c6a Compare April 5, 2019 13:46
Software `source` method now supports the `authorization` option.
This allows the user to supply an HTTP Authorization header which will
be passed to the source URL.

For the common case of basic authentication, that looks like this:

```
require 'base64'

username = 'XXX'
password = 'YYY'
credentials = Base64.encode64("#{username}:#{password}")

version 'the_version' do
  source url: 'https://example.com/path/to/the/file',
         authorization: "Basic #{credentials}",
         ...
end
```

Signed-off-by: Lee Green <[email protected]>
@LeeXGreen LeeXGreen force-pushed the netfetcher_http_authorization branch from 8559c6a to 55018e8 Compare April 5, 2019 13:50
@LeeXGreen LeeXGreen force-pushed the netfetcher_http_authorization branch from c66a474 to 6bf8a0f Compare April 5, 2019 14:40
@LeeXGreen
Copy link
Contributor Author

Let me know what you think about this test -- I didn't see any similar test to copy from. I don't love the use of send to call download, but that seemed better than mocking out all the extra machinery in fetch.

@marcparadise
Copy link
Member

Thanks @LeeXGreen . I think the tests are good - using :send is not ideal, but it does the right thing without having to mock internals of things we don't care about - which is 👍

@marcparadise
Copy link
Member

Looks like we need one more rebase when you have a chance, otherwise this looks good to go.

Copy link

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

👍

... with a suggestion for updating the documentation for the new option with an example for people with bad memories like me.

@@ -263,6 +263,8 @@ def dependency(val)
#
# @option val [String] :cookie (nil)
# a cookie to set
# @option val [String] :authorization (nil)
# an authorization header to set

Choose a reason for hiding this comment

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

Any chance to update this documentation with the example? I'd have to scrape the barrel of my web header knowledge to remember how to set this. Maybe something like this?

Suggested change
# an authorization header to set
# an authorization header to set, e.g. "Basic " + Base64.encode64("#{username}:#{password}")

@lamont-granquist
Copy link
Contributor

@LeeXGreen there's a pile of subject.send(:whatever) already. i think it is probably fine and is either indicative that ruby lacks appropriate protection levels or else that the object in some ideal world needs some massive refactoring that will never happen.

@lamont-granquist lamont-granquist merged commit b9f8e54 into chef:master Apr 16, 2019
@robbkidd
Copy link

Change appears in v6.0.25.

(Arguably should have bumped a minor there, but 🤷‍♂️.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Waiting on Contributor A pull request that has unresolved requested actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants