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

TLS auth cannot be used between proxy and brokers #1991

Closed
ivankelly opened this issue Jun 19, 2018 · 15 comments
Closed

TLS auth cannot be used between proxy and brokers #1991

ivankelly opened this issue Jun 19, 2018 · 15 comments
Assignees
Labels
area/broker area/proxy type/bug The PR fixed a bug or issue reported a bug

Comments

@ivankelly
Copy link
Contributor

ivankelly commented Jun 19, 2018

Brokers are configured to use TLS authentication

Proxies cannot connect to the TLS authentication required brokers, because the brokers are advertising their address as :8080, not :8443

@ivankelly ivankelly added the type/bug The PR fixed a bug or issue reported a bug label Jun 19, 2018
@ivankelly
Copy link
Contributor Author

My original hunch on the advertising being wrong was incorrect. It does in fact go to the correct address if you have the right set of options configured.

my proxy.conf tls bits

authenticationEnabled=true
authenticationProviders=org.apache.pulsar.broker.authentication.AuthenticationProviderTls
authorizationEnabled=true
brokerClientAuthenticationPlugin=org.apache.pulsar.client.impl.auth.AuthenticationTls
brokerClientAuthenticationParameters=tlsCertFile:/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/client-keys/admin.cert.pem,tlsKeyFile:/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/client-keys/admin.key-pk8.pem
brokerClientTrustCertsFilePath=/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/certs/ca.cert.pem                                                                                                               
tlsEnabledInProxy=true
tlsEnabledWithBroker=true
tlsCertificateFilePath=/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/server-keys/broker.cert.pem
tlsKeyFilePath=/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/server-keys/broker.key-pk8.pem
tlsTrustCertsFilePath=/home/ivan/src/pulsar/checkouts/testing-ca/tests/certificate-authority/certs/ca.cert.pem
tlsAllowInsecureConnection=true

my broker.conf tls bits

tlsEnabled=true
tlsCertificateFilePath=/pulsar/ssl/broker.cert.pem
tlsKeyFilePath=/pulsar/ssl/broker.key-pk8.pem
tlsTrustCertsFilePath=/pulsar/ssl/ca.cert.pem
authenticationEnabled=true
authenticationProviders=org.apache.pulsar.broker.authentication.AuthenticationProviderTls
superUserRoles=admin
authorizationEnabled=true

tlsAllowInsecureConnection doesn't exist in the config file as is. It needs to be documented.

This works for the brokerService port. I have another role "ivan".
If "ivan" tries to produce to public/default/topic1 via the proxy or broker, he cannot. If the admin user grants "ivan" produce action permissions on the public/default namespace, then he can write.

However, if "ivan" uses they admin client against the proxy, he can grant himself any permissions he wants.

This is because the proxy is authenticated against the broker as a superuser, anyone authenticated against the proxy gains those superuser powers. There's no forwarding of principal from the proxy for admin like there is for data.

ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 22, 2018
So that they can be configured to run with TLS.

The certificate authority used to generate the certs is also included
in tests/ with instructions on how to generate new user and server
certs.

These certs will be used for an integration test once apache#1991 is solved.
sijie pushed a commit that referenced this issue Jun 22, 2018
So that they can be configured to run with TLS.

The certificate authority used to generate the certs is also included
in tests/ with instructions on how to generate new user and server
certs.

These certs will be used for an integration test once #1991 is solved.
@pckeyan
Copy link
Contributor

pckeyan commented Jun 22, 2018

@ivankelly @sijie Is this not an issue? Is this tested in K8s along with nodeport config?

@ivankelly
Copy link
Contributor Author

@pckeyan This is an issue, yes. Working on a fix now, though I'm testing directly in docker locally. Will add a task for testing with nodeport in k8s also.

@ivankelly
Copy link
Contributor Author

@jai1 @merlimat I've been looking at how proxyRoles and originalPrincipal work with the data protocol. Is it correct to say that, with this mechanism, you need to explicitly grant produce and consume to the proxy role on every individual namespace that you want accessible through the proxy?

@pckeyan
Copy link
Contributor

pckeyan commented Jun 25, 2018

@ivankelly I tested with the identified parameter, SSL is working is fine between Proxy and Broker in K8s. Thanks for your help. Appreciated.

@ivankelly
Copy link
Contributor Author

@pckeyan do you have authorization enabled? The problem with the parameters I put is that any client that is authorized with the proxy will have full superuser access for admin operations, which obviously isn't ideal.

@pckeyan
Copy link
Contributor

pckeyan commented Jun 25, 2018

Yes I have authorization enabled, I tested across LOBs of my use case and is rightly declined. Let me retest. Let me know if you want me to test some thing different. I have billing and order as two lobs. Billing owned topic cannot be used by Order to produce/consume.

@ivankelly
Copy link
Contributor Author

produce/consume isn't the problem. The problem is that someone authorized as bank can use the admin api to grant permissions to themselves to produce/consume on a card owned topic.

@pckeyan
Copy link
Contributor

pckeyan commented Jun 25, 2018

But still they need Certificates to pass it on to get authenticated; Please correct my understanding

@ivankelly
Copy link
Contributor Author

@pckeyan yes, it's not wide open, but it's more open than it should be.

@pckeyan
Copy link
Contributor

pckeyan commented Jun 25, 2018

@ivankelly Are you planning for a fix there? What would be ideal fix?

@ivankelly
Copy link
Contributor Author

@pckeyan I'd like to talk to @merlimat & @jai1 since they've worked on similar stuff, but the planned fix is to send the original authenticated rolename with the proxied requests, similiar to how we do for the data protocol. Then we'd need to document all this stuff, because there's no documentation right now. Hopefully I can knock out the fix tomorrow.

@jai1
Copy link
Contributor

jai1 commented Jun 26, 2018 via email

@ivankelly
Copy link
Contributor Author

ivankelly commented Jun 26, 2018

" forwardAuthorizationCredentials" on the proxy and enable " authenticateOriginalAuthData"

I assume this only applies to Athenz, as TLS doesn't use the auth data AFAICS.

This weekend I will go through the documentation and add any missing pieces (if any)

There's quite a bit missing, in particular for the TLS auth context, which most people will be using. Once I've solved this issue, I'll take a pass over the docs too.

In terms of this issue, my plan is to make the admin api respect originalPrincipal and proxyRoles also, carrying it in a header ("X-Original-Principal").

This does mean that proxy would have to be added as admin role for each tenant that wants to use admin api through the proxy. In some usecases though, I think proxy will have to be a superuser at the broker.

ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 26, 2018
The broker admin checks if the authenticated user is listed as a proxy
user. If so, it checks looks for the header X-Original-Principal, and
validates that the role is authorized to access the resource in
question.

There are two use cases:

1. The proxy role is a normal role. In this case, if a resource is to
be used via the proxy, the proxy user must be explicitly granted
permission on the resource. So, to use the admin api for a tenant, the
proxy role must be listed as a tenant admin.
2. The proxy role is a superuser role. In this case, any resource can
be used via the proxy as long as the user authorized with the proxy is
authorized to use the resource. However, if the proxy is compromised,
a bad actor has full access to the cluster.

This patch is the first part of a fix for apache#1991.
ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 27, 2018
The broker admin checks if the authenticated user is listed as a proxy
user. If so, it checks looks for the header X-Original-Principal, and
validates that the role is authorized to access the resource in
question.

There are two use cases:

1. The proxy role is a normal role. In this case, if a resource is to
be used via the proxy, the proxy user must be explicitly granted
permission on the resource. So, to use the admin api for a tenant, the
proxy role must be listed as a tenant admin.
2. The proxy role is a superuser role. In this case, any resource can
be used via the proxy as long as the user authorized with the proxy is
authorized to use the resource. However, if the proxy is compromised,
a bad actor has full access to the cluster.

This patch is the first part of a fix for apache#1991.
ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 27, 2018
It turns out that the admin rest api proxy wasn't doing authentication
at all, so if you can connect, you have access to the api at the same
level as the configured user of the proxy, which could well be
superuser access.

Master Issue: apache#1991
ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 27, 2018
So that the same authentication service can be used for the webserver also.

Master issue: apache#1991
ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 27, 2018
So that it can be used by the proxy also.

Master issue: apache#1991
merlimat pushed a commit that referenced this issue Jun 27, 2018
So that it can be used by the proxy also.

Master issue: #1991
sijie pushed a commit that referenced this issue Jun 28, 2018
So that the same authentication service can be used for the webserver also.

Master issue: #1991
sijie pushed a commit that referenced this issue Jun 28, 2018
* Original principal authorization for admin API

The broker admin checks if the authenticated user is listed as a proxy
user. If so, it checks looks for the header X-Original-Principal, and
validates that the role is authorized to access the resource in
question.

There are two use cases:

1. The proxy role is a normal role. In this case, if a resource is to
be used via the proxy, the proxy user must be explicitly granted
permission on the resource. So, to use the admin api for a tenant, the
proxy role must be listed as a tenant admin.
2. The proxy role is a superuser role. In this case, any resource can
be used via the proxy as long as the user authorized with the proxy is
authorized to use the resource. However, if the proxy is compromised,
a bad actor has full access to the cluster.

This patch is the first part of a fix for #1991.

* Factor out some validation for original principal

Also added a check that if original principal is set, it must have
come from a role contained in proxyRoles.
sijie pushed a commit that referenced this issue Jun 28, 2018
So that it can be used by the proxy also.

Master issue: #1991
ivankelly added a commit to ivankelly/pulsar that referenced this issue Jun 30, 2018
It turns out that the admin rest api proxy wasn't doing authentication
at all, so if you can connect, you have access to the api at the same
level as the configured user of the proxy, which could well be
superuser access.

Master Issue: apache#1991
sijie pushed a commit that referenced this issue Jul 12, 2018
It turns out that the admin rest api proxy wasn't doing authentication
at all, so if you can connect, you have access to the api at the same
level as the configured user of the proxy, which could well be
superuser access.

Master Issue: #1991
@sijie
Copy link
Member

sijie commented Sep 12, 2018

This has been fixed by multiple pull requests linked above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker area/proxy type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

No branches or pull requests

4 participants