You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "michaeljmarshall (via GitHub)" <gi...@apache.org> on 2023/02/17 23:37:45 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19557: [fix][proxy] Only add X-Original-Principal when necessary
michaeljmarshall opened a new pull request, #19557:
URL: https://github.com/apache/pulsar/pull/19557
### Motivation
I broker 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 the proxy by changing the way that the proxy uses the `X-Original-Principal` header. The primary benefit will be a reduction in unnecessary authorization checks.
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.
In this PR, I propose that we only add the `X-Original-Principal` when we are using the proxy's authentication.
### Modifications
* Only add the `X-Original-Principal` header when the proxy is authenticating with the broker via TLS.
### 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 https://github.com/apache/pulsar/pull/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: https://github.com/michaeljmarshall/pulsar/pull/31
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall merged pull request #19557: [fix][broker] Allow proxy to pass same role for authRole and originalRole
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall merged PR #19557:
URL: https://github.com/apache/pulsar/pull/19557
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19557: [fix][broker] Allow proxy to pass same role for authRole and originalRole
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19557:
URL: https://github.com/apache/pulsar/pull/19557#issuecomment-1439079583
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19557: [fix][broker] Allow proxy to pass same role for authRole and originalRole
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19557:
URL: https://github.com/apache/pulsar/pull/19557#issuecomment-1516809345
After reviewing this, I realize now that the only place I needed to add docs was in the Javadocs. We don't document the HTTP proxy to broker interactions on our website, so nothing needs to be updated there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19557: [fix][broker] Allow proxy to pass same role for authRole and originalRole
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19557:
URL: https://github.com/apache/pulsar/pull/19557#issuecomment-1439521165
The flaky proxy test is fixed by https://github.com/apache/pulsar/pull/19594
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] momo-jun commented on pull request #19557: [fix][broker] Allow proxy to pass same role for authRole and originalRole
Posted by "momo-jun (via GitHub)" <gi...@apache.org>.
momo-jun commented on PR #19557:
URL: https://github.com/apache/pulsar/pull/19557#issuecomment-1445774465
Hi @michaeljmarshall , I'm checking in to follow up with the doc updates. Did you have any plans to update the docs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org