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

also allow whitelisted admin clients to clean certs #748

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

foxxx0
Copy link
Contributor

@foxxx0 foxxx0 commented Jun 2, 2020

During #728 a regression was introduced, denying the other whitelisted
admin clients cleaning/deletion of certificates:

2020-06-02T16:30:47.856+02:00 ERROR [qtp1105504743-114201] [p.t.a.rules] Forbidden request: puppetserver01.[...] access to /puppet-ca/v1/certificate_status/my.fancy.hostname (method :delete) (authenticated: true) denied by rule 'Allow nodes to delete their own certificates'.

The solution is to re-allow the entries within
@server_admin_api_whitelist, which usually contain "localhost" and the
fqdn of the puppetserver CA system.

Unfortunately I'm unable to run the test suite, it just fails pretty much everywhere which is unrelated to my changes.

I have deployed this patch in our infra and can confirm it fixes the auth deny issue when trying to clean certs locally on the puppetserver CA machine with $server_ca_client_self_delete enabled.

During theforeman#728 a regression was introduced, denying the other whitelisted
admin clients cleaning/deletion of certificates:

```
2020-06-02T16:30:47.856+02:00 ERROR [qtp1105504743-114201] [p.t.a.rules] Forbidden request: puppetserver01.[...] access to /puppet-ca/v1/certificate_status/my.fancy.hostname (method :delete) (authenticated: true) denied by rule 'Allow nodes to delete their own certificates'.
```

The solution is to re-allow the entries within
`@server_admin_api_whitelist`, which usually contain "localhost" and the
fqdn of the puppetserver CA system.
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks correct to me. I'll let the test suite continue but other than that I think it can be merged.

@foxxx0
Copy link
Contributor Author

foxxx0 commented Jun 2, 2020

all green \o/

@ekohl ekohl merged commit f63fdaa into theforeman:master Jun 10, 2020
@foxxx0 foxxx0 deleted the fix-ca-clean branch June 15, 2020 19:33
@alexjfisher
Copy link
Contributor

Is this definitely the right solution? This rule still seems to conflict with

{
# Allow the CA CLI to access the certificate_status endpoint
match-request: {
path: "/puppet-ca/v1/certificate_status"
type: path
method: [get, put, delete]
}
<%- if @server_ca_auth_required == false -%>
allow-unauthenticated: true
<%- else -%>
allow: [
<%- @server_ca_client_whitelist.each do |client| -%>
"<%= client %>",
<%- end -%>
{
extensions: {
pp_cli_auth: "true"
}
}
]
<%- end -%>
sort-order: 500
name: "puppetlabs cert status"
},

Shouldn't the endpoint still have been for server_ca_client_whitelist? These clients still can't delete certificates and meanwhile admin api clients (which previously could do nothing destructive) now can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants