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

(SERVER-763) Read client auth info from tk-authz when configured #709

Conversation

@camlow325
Copy link
Contributor

camlow325 commented Sep 28, 2015

This commit changes the Ruby request handler to read client
authentication info from trapperkeeper-authorization when tk-authz is
enabled via the use-legacy-auth-conf: false setting. Support for the
pre-existing method of getting this info - via X-Client headers or an
SSL certificate, depending upon how the master.allow-header-cert-info
setting is configured - is preserved in the case that no information can
be derived from tk-authz. tk-authz is presumed to provide client
authentication info via an authorization map within the Ring request
map.

@camlow325

This comment has been minimized.

Copy link
Contributor Author

camlow325 commented Sep 28, 2015

This won't pass Travis CI yet and isn't yet ready to merge because it depends upon a snapshot version of trapperkeeper-authorization. In order for this PR to be ready to merge, We'll need to have merged puppetlabs/trapperkeeper-authorization#17 and done a release of trapperkeeper-authorization that this PR can be bumped up to. I've put this PR up in the meantime for early review.

Depending upon how long it takes for PR reviews to take place, I may add some additional docs-related commits with some updates to the language related to the allow-header-cert-info feature and tk-auth integration as well. I'll add comments to this PR as I add any new docs commits.

@camlow325 camlow325 changed the title (SERVER-763) Read client auth info from tk-authz when configured (FOR REVIEW ONLY) (SERVER-763) Read client auth info from tk-authz when configured Sep 28, 2015
@camlow325

This comment has been minimized.

Copy link
Contributor Author

camlow325 commented Sep 28, 2015

Added some updates to the configuration.markdown and external_ssl_termination.markdown docs in a separate commit - 5550e07.

camlow325 added 3 commits Sep 24, 2015
This commit changes the Ruby request handler to read client
authentication info from trapperkeeper-authorization when tk-authz is
enabled via the `use-legacy-auth-conf: false` setting.  Support for the
pre-existing method of getting this info - via X-Client headers or an
SSL certificate, depending upon how the `master.allow-header-cert-info`
setting is configured - is preserved in the case that no information can
be derived from tk-authz.  tk-authz is presumed to provide client
authentication info via an `authorization` map within the Ring request
map.
This commit contains some updates to the `configuration.markdown` and
`external_ssl_termination.markdown` docs.  The updates pertain to new /
deprecated settings in Puppet Server per the integration of support for
trapperkeeper-authorization.
… helpers

This commit updates Puppet Server's trapperkeeper-authorization
dependency to 0.1.4 and updates some code to use some Ring helpers for
extracting authenticated client info.
@camlow325 camlow325 force-pushed the camlow325:task/master/SERVER-763-adapt-tk-authz-header-cert-info branch from 5550e07 to d12449b Sep 29, 2015
@camlow325

This comment has been minimized.

Copy link
Contributor Author

camlow325 commented Sep 29, 2015

Rebased and updated to trapperkeeper-authorization 0.1.4 in d12449b. Should be ready to review for possible merge now.

@camlow325 camlow325 changed the title (FOR REVIEW ONLY) (SERVER-763) Read client auth info from tk-authz when configured (SERVER-763) Read client auth info from tk-authz when configured Sep 29, 2015

Note that, for backward compatibility, the values of other configuration
settings control the specific endpoints for which `trapperkeeper-authorization`
is usedr:

This comment has been minimized.

Copy link
@nwolfe

nwolfe Sep 29, 2015

Contributor

s/usedr/used

(log/errorf "The DN '%s' provided by the HTTP header '%s' is malformed."
(let [cn (ssl-utils/x500-name->CN header-dn-val)
authenticated (= "SUCCESS" header-auth-val)]
(log/debugf "CN '%s' provided by HTTP header '%s'"

This comment has been minimized.

Copy link
@dankreek

dankreek Sep 29, 2015

Contributor

Logging++

For a value of `true`, also the default value if not specified, authorization
will be done in core Ruby puppet-agent code via the legacy
[`auth.conf`](https://docs.puppetlabs.com/puppet/4.2/reference/config_file_auth.html)
file. For a value of `false`, authorization will be done through via

This comment has been minimized.

Copy link
@nwolfe

nwolfe Sep 29, 2015

Contributor

s/through via/via

{header-dn-name header-dn-val
header-auth-name header-auth-val
header-client-cert-name header-cert-val}]
(if header-val

This comment has been minimized.

Copy link
@nwolfe

nwolfe Sep 30, 2015

Contributor

All the comprehension forms have special support for the common conditionals that follow, so you could collapse this if into the doseq like so:

(doseq [[header-name header-val] {...}
        :when header-val]
...)

Might be worthwhile in this particular function with as many nesting levels as it has.

@nwolfe

This comment has been minimized.

Copy link
Contributor

nwolfe commented Sep 30, 2015

👍 other than minor comments

This commit addresses some feedback on the associated PR:

* Fixes a couple of typos
* Pulls a when conditional up from a child form into its parent doseq
@camlow325 camlow325 force-pushed the camlow325:task/master/SERVER-763-adapt-tk-authz-header-cert-info branch from 756eebb to 9f79699 Sep 30, 2015
@camlow325

This comment has been minimized.

Copy link
Contributor Author

camlow325 commented Sep 30, 2015

@nwolfe Addressed your PR feedback - typos and the doseq/when thing - in 9f79699.

{:client-cert (ring-auth/authorized-certificate request)
:client-cert-cn (ring-auth/authorized-name request)
:authenticated (true? (ring-auth/authorized-authentic? request))}
(let [headers (:headers request)

This comment has been minimized.

Copy link
@dankreek

dankreek Sep 30, 2015

Contributor

ring actually had a get-header function which allows you to grab headers by name directly from the request object, but this is totally fine with me as it is.

@dankreek

This comment has been minimized.

Copy link
Contributor

dankreek commented Sep 30, 2015

👍 Looks good, just had that one little "actually" comment that's not even remotely a blocker here

dankreek added a commit that referenced this pull request Sep 30, 2015
…k-authz-header-cert-info

(SERVER-763) Read client auth info from tk-authz when configured
@dankreek dankreek merged commit 69e80d9 into puppetlabs:master Sep 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.