Modify authz tests of GET_TOPICS and PACKAGES op #12771
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RocMarshal
approved these changes
Nov 12, 2021
Contributor
RocMarshal
left a comment
RocMarshal
left a comment
There was a problem hiding this comment.
Good job. @yuruguo Thanks for the contribution. From my perspective this looks good. I also like to get concise in coding.
RobertIndie
approved these changes
Nov 12, 2021
RobertIndie
reviewed
Nov 12, 2021
Member
RobertIndie
left a comment
RobertIndie
left a comment
There was a problem hiding this comment.
I think we should remove the test class here:
codelipenghui
approved these changes
Nov 15, 2021
eolivelli
pushed a commit
to eolivelli/pulsar
that referenced
this pull request
Nov 29, 2021
### Motivation Integration authz test of [GetTopicsOfNamespaceWithAuthTest.java](https://github.com/apache/pulsar/blob/master/tests/integration/src/test/java/org/apache/pulsar/tests/integration/auth/admin/GetTopicsOfNamespaceWithAuthTest.java) and [PackagesOpsWithAuthTest.java](https://github.com/apache/pulsar/blob/master/tests/integration/src/test/java/org/apache/pulsar/tests/integration/auth/admin/PackagesOpsWithAuthTest.java) is too much heavyweight for check GET_TOPICS and PACKAGES namespace op. In fact, we can add a small amount of code in [AuthorizationProducerConsumerTest.java](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java) to replace with above integration test.
fxbing
pushed a commit
to fxbing/pulsar
that referenced
this pull request
Dec 19, 2021
### Motivation Integration authz test of [GetTopicsOfNamespaceWithAuthTest.java](https://github.com/apache/pulsar/blob/master/tests/integration/src/test/java/org/apache/pulsar/tests/integration/auth/admin/GetTopicsOfNamespaceWithAuthTest.java) and [PackagesOpsWithAuthTest.java](https://github.com/apache/pulsar/blob/master/tests/integration/src/test/java/org/apache/pulsar/tests/integration/auth/admin/PackagesOpsWithAuthTest.java) is too much heavyweight for check GET_TOPICS and PACKAGES namespace op. In fact, we can add a small amount of code in [AuthorizationProducerConsumerTest.java](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java) to replace with above integration test.
1 task
michaeljmarshall
added a commit
that referenced
this pull request
Feb 22, 2023
…Role (#19557) ### Motivation I broke the Pulsar Proxy with #19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with #19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. ### Modifications * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. ### Alternative Solutions I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. ### Verifying this change When cherry-picking #19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by #12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. ### Does this pull request potentially affect one of the following parts: This is not a breaking change. ### Documentation - [x] `doc-required` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#31
michaeljmarshall
added a commit
that referenced
this pull request
Feb 22, 2023
…Role (#19557) I broke the Pulsar Proxy with #19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with #19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking #19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by #12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: michaeljmarshall#31 (cherry picked from commit d4be954)
Member
|
Just want to mention that this test re-write removed test coverage for admin client to proxy to broker with different kinds of authentication. I fixed it here #19557. |
michaeljmarshall
added a commit
that referenced
this pull request
Feb 22, 2023
…Role (#19557) I broke the Pulsar Proxy with #19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with #19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking #19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by #12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: michaeljmarshall#31 (cherry picked from commit d4be954) (cherry picked from commit 5f5551d)
michaeljmarshall
added a commit
to michaeljmarshall/pulsar
that referenced
this pull request
Feb 22, 2023
…Role (apache#19557) I broke the Pulsar Proxy with apache#19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with apache#19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking apache#19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by apache#12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: #31 (cherry picked from commit d4be954) (cherry picked from commit 5f5551d) (cherry picked from commit 4da2487)
michaeljmarshall
added a commit
to datastax/pulsar
that referenced
this pull request
Feb 23, 2023
…Role (apache#19557) I broke the Pulsar Proxy with apache#19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with apache#19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking apache#19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by apache#12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: michaeljmarshall#31 (cherry picked from commit d4be954) (cherry picked from commit 5f5551d) (cherry picked from commit 4da2487)
michaeljmarshall
added a commit
to michaeljmarshall/pulsar
that referenced
this pull request
Feb 23, 2023
…Role (apache#19557) I broke the Pulsar Proxy with apache#19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with apache#19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking apache#19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by apache#12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: #31 (cherry picked from commit d4be954) (cherry picked from commit 5f5551d) (cherry picked from commit 4da2487) (cherry picked from commit dc09681)
michaeljmarshall
added a commit
to michaeljmarshall/pulsar
that referenced
this pull request
Feb 23, 2023
…Role (apache#19557) I broke the Pulsar Proxy with apache#19455 because that PR requires that when `X-Original-Principal` is supplied, the auth role must be a proxy role. This is not always the case for proxied admin requests. This PR seeks to fix that incorrect assumption by changing the way verification is done for the roles. Specifically, when the two roles are the same and they are not a proxy role, we will consider it a valid combination. Note that there is no inefficiency in this solution because When the `authenticatedPrincipal` is not a proxy role, that is the only role that is authenticated. Note also that we do not let the binary protocol authenticate this way, and that is consistent with the way the pulsar proxy forwards authentication data. Currently, we do the following when authentication is enabled in the proxy: 1. Authenticate the client's http request and put the resulting role in the `X-Original-Principal` header for the call to the broker. https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L370-L373 2. Copy the `Authorization` header into the broker's http request: https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L232-L236 3. Configure the proxy's http client to use client TLS authentication (when configured): https://github.com/apache/pulsar/blob/38555851359f9cfc172650c387a58c5a03809e97/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java#L269-L277 The problem with apache#19455 is that it assumes the proxy supplies its own authentication data. However, that only happens when using TLS authentication. Otherwise, the proxy forwards the client's authentication data in the `Authorization` header. As such, calls will fail because the `X-Original-Principal` header supplied without using a proxy role. * Consider the `authenticatedPrincipal` and the `originalPrincipal` a valid pair when they are equal and are not a `proxyRole` for http requests. I initially proposed that we only add the `X-Original-Principal` when we are using the proxy's authentication (see the first commit). I decided this solution is not ideal because it doesn't solve the problem, it doesn't make the brokers backwards compatible, and there isn't actually any inefficiency in passing the role as a header. When cherry-picking apache#19455 to branch-2.9, I discovered that `PackagesOpsWithAuthTest#testPackagesOps` was consistently failing because of the way the proxy supplies authentication data when proxying http requests. That test was removed by apache#12771, which explains why I didn't catch the error sooner. This PR includes a test that fails without this change. Note that the primary issue must be that we didn't have any tests doing authentication forwarding through the proxy. Now we will have both relevant tests where the proxy is and is not authenticating. This is not a breaking change. - [x] `doc-required` PR in forked repository: #31 (cherry picked from commit d4be954) (cherry picked from commit 5f5551d) (cherry picked from commit 4da2487) (cherry picked from commit dc09681)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Integration authz test of GetTopicsOfNamespaceWithAuthTest.java and PackagesOpsWithAuthTest.java is too much heavyweight for check GET_TOPICS and PACKAGES namespace op.
In fact, we can add a small amount of code in AuthorizationProducerConsumerTest.java to replace with above integration test.
Documentation
no-need-doc