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/07 20:25:22 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
michaeljmarshall opened a new pull request, #19455:
URL: https://github.com/apache/pulsar/pull/19455
### Motivation
While working on https://github.com/apache/pulsar/pull/19270, I noticed we do not set any strict rules on which roles can supply the `originalPrincipal` or `originalAuthData` fields on the `ConnectCommand`:
https://github.com/apache/pulsar/blob/b0945d1d45d1e911d24151a23cf284e476203ba7/pulsar-common/src/main/proto/PulsarApi.proto#L279-L288
The goal is to prevent connections where the `originalPrincipal` is set with an `authRole` that is not a proxy role. This is an invalid state that was allowed to persist before. It was not, strictly speaking, a vulnerability because the `isTopicOperationAllowed` validates that both the `originalPrincipal` and the `authRole` have permission to perform any operation the client attempts.
This is technically a breaking change in that upgrading existing proxies will not work if the `proxyRoles` is not correctly configured in the `broker.conf`.
### Modifications
* Update the `ServerCnx#isValidRoleAndOriginalPrincipal` method to require that when `originalPrincipal` is set, the `authRole` must be in the `proxyRoles` set. Because we run this check after authenticating the `originalAuthData`, we will correctly fail calls that pass and do not pass the `originalAuthData`.
### Verifying this change
An existing test is updated to cover the added logic.
### Does this pull request potentially affect one of the following parts:
- [x] The binary protocol
This change affects the binary protocol's usage without changing the binary protocol itself.
### Documentation
- [x] `doc-required`
If we accept this change, I'll follow up with a docs PR to make sure all documentation is up to date.
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/25
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1423349013
/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] nodece commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1422000748
Good work!
> This is technically a breaking change in that upgrading existing proxies will not work if the `proxyRoles` is not correctly configured in the `broker.conf`.
I think we should add a paramter in the config file, and when this paramter is enabled, we perform the strict 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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1463223990
> This change is always positive, but we shouldn't affect the old version because many users don't care about the `proxyRoles`. Once the user upgrades the Pulsar version, this user gets an error about the proxy role.
I do not think we should revert this change from old branches. The risk to users is that misconfiguration leads to excessive permissions, as I documented. The solution is to use dedicated authentication data for a proxy so that it has a proxy role. I will reply on the mailing list as well.
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1423355434
@lhotari - any idea on how to get around these failures?
--
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] nodece commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1463249297
I understand that, but for the sake of the user, I think this check is optional, and we can add a switch to control this check, then we update our documentation that how to enable this switch.
Default to enable this switch in the 3.0.0, disable this switch in the 2.8, 2.9, 2.10, 2.11.
--
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] tuteng commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "tuteng (via GitHub)" <gi...@apache.org>.
tuteng commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1463376940
> @tuteng - the proxy is supposed to connect using one of the `proxyRoles` configured in the `broker.conf`. This is somewhat documented here https://pulsar.apache.org/docs/2.11.x/security-authorization/#proxy-roles. If the proxy connects with something other than a proxy role when using TLS authentication, only the proxy's role will be used to verify authorization, and that will likely result in accidental elevation of privileges. In my opinion, this is not a breaking change because it is preventing a state that shouldn't have existed in the first place. However, I guess that conclusion is up for debate.
I would like to confirm if it is possible to make it optional (enabled or disabled), in many user environments it can be trusted and there is no such risk, now is it a good option to have it on by default in all major versions and not be disabled? @michaeljmarshall
--
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] nodece commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1462241088
This change is always positive, but we shouldn't affect the old version because many users don't care about the `proxyRoles`. Once the user upgrades the Pulsar version, this user gets an error about the proxy role.
I think this change can only be released in 3.0.
If you agree, please help revert this PR in the old branch!
--
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] lhotari commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1425432593
This reproduces consistently for me locally. Error message was "No subject alternative DNS name matching pop-os.localdomain found." Perhaps in your local environment, the default hostname is "localhost" or "127.0.0.1". On Linux, it's usually something else than that. I guess this is where MacOSX differs.
Full error
```
2023-02-10T10:44:51,221 - WARN - [pulsar-client-io-49-3:ClientCnx@280] - Error during handshake
javax.net.ssl.SSLHandshakeException: General OpenSslEngine problem
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.handshakeException(ReferenceCountedOpenSslEngine.java:1907) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.wrap(ReferenceCountedOpenSslEngine.java:834) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at javax.net.ssl.SSLEngine.wrap(SSLEngine.java:564) ~[?:?]
at io.netty.handler.ssl.SslHandler.wrap(SslHandler.java:1041) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.wrapNonAppData(SslHandler.java:927) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1409) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.decodeNonJdkCompatible(SslHandler.java:1247) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1287) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:800) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:499) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:397) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at java.lang.Thread.run(Thread.java:833) ~[?:?]
Caused by: java.security.cert.CertificateException: No subject alternative DNS name matching pop-os.localdomain found.
at sun.security.util.HostnameChecker.matchDNS(HostnameChecker.java:212) ~[?:?]
at sun.security.util.HostnameChecker.match(HostnameChecker.java:103) ~[?:?]
at sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:458) ~[?:?]
at sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:418) ~[?:?]
at sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:292) ~[?:?]
at sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:144) ~[?:?]
at io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:234) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:779) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.internal.tcnative.CertificateVerifierTask.runTask(CertificateVerifierTask.java:36) ~[netty-tcnative-classes-2.0.56.Final.jar:2.0.56.Final]
at io.netty.internal.tcnative.SSLTask.run(SSLTask.java:48) ~[netty-tcnative-classes-2.0.56.Final.jar:2.0.56.Final]
at io.netty.internal.tcnative.SSLTask.run(SSLTask.java:42) ~[netty-tcnative-classes-2.0.56.Final.jar:2.0.56.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.runAndResetNeedTask(ReferenceCountedOpenSslEngine.java:1496) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.access$700(ReferenceCountedOpenSslEngine.java:94) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine$TaskDecorator.run(ReferenceCountedOpenSslEngine.java:1471) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.runDelegatedTasks(SslHandler.java:1549) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1395) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
... 19 more
2023-02-10T10:44:51,228 - WARN - [pulsar-proxy-io-37-2:DefaultChannelPipeline@1152] - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
io.netty.handler.codec.DecoderException: javax.net.ssl.SSLHandshakeException: error:10000438:SSL routines:OPENSSL_internal:TLSV1_ALERT_INTERNAL_ERROR
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:499) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.flush.FlushConsolidationHandler.channelRead(FlushConsolidationHandler.java:152) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) ~[netty-transport-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:800) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:499) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:397) ~[netty-transport-classes-epoll-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.87.Final.jar:4.1.87.Final]
at java.lang.Thread.run(Thread.java:833) ~[?:?]
Caused by: javax.net.ssl.SSLHandshakeException: error:10000438:SSL routines:OPENSSL_internal:TLSV1_ALERT_INTERNAL_ERROR
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.shutdownWithError(ReferenceCountedOpenSslEngine.java:1086) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.sslReadErrorResult(ReferenceCountedOpenSslEngine.java:1377) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1317) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1404) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1447) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler$SslEngineType$1.unwrap(SslHandler.java:222) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1343) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.decodeNonJdkCompatible(SslHandler.java:1247) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1287) ~[netty-handler-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468) ~[netty-codec-4.1.87.Final.jar:4.1.87.Final]
... 19 more
```
here's the fix: ea2b07d56d89b2b66b5e5ad7b8ad10da0357ac69 .
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1427397180
Thanks for taking a look @lhotari! That fixed some of the errors, but I found another one that was my own fault. Analysis follows:
Got this error again:
```
Error: testAuthorizedUserAsOriginalPrincipal(org.apache.pulsar.client.impl.AdminApiKeyStoreTlsAuthTest) Time elapsed: 0.203 s <<< FAILURE!
org.apache.pulsar.client.admin.PulsarAdminException: java.util.concurrent.CompletionException: org.apache.pulsar.client.admin.internal.http.AsyncHttpConnector$RetryException: Could not complete the operation. Number of retries has been exhausted. Failed reason: Received fatal alert: certificate_unknown
```
After digging into it a bit more, I discovered the issue was introduced 16a7cc04cb89fed083c37b8e17287949535d4940. The problem was pretty subtle. I accidentally generated a new client cert and put that cert in the `proxy-and-client.truststore.jks`. However, I didn't commit the updated `client.keystore.jks`, so I ended up with a mismatch.
My first mistake was assuming that `certificate_unknown` was a client error. I should have realized that the `Number of retries has been exhausted` actually pointed at a server side error. Specifically, the server didn't trust the certificate.
This test was passing locally because I had the correct client private key locally, but I had chosen (poorly) to not commit it.
Also, I made things "worse" by disable insecure TLS connections. With my latest commit, the tests in `AdminApiKeyStoreTlsAuthTest` should pass.
--
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] nodece commented on a diff in pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#discussion_r1104272112
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java:
##########
@@ -150,19 +150,11 @@ public static boolean isClientAuthenticated(String appId) {
return appId != null;
}
- private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
- String originalPrincipal) {
- if (proxyRoles.contains(authenticatedPrincipal)) {
- // Request has come from a proxy
- if (StringUtils.isBlank(originalPrincipal)) {
- log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
- throw new RestException(Status.UNAUTHORIZED,
- "Original principal cannot be empty if the request is via proxy.");
- }
- if (proxyRoles.contains(originalPrincipal)) {
- log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
- throw new RestException(Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
- }
+ private void validateOriginalPrincipal(String authenticatedPrincipal, String originalPrincipal) {
+ if (!pulsar.getBrokerService().getAuthorizationService()
+ .isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, clientAuthData())) {
+ throw new RestException(Status.UNAUTHORIZED,
Review Comment:
Or `BAD_REQUEST`?
--
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] tuteng commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "tuteng (via GitHub)" <gi...@apache.org>.
tuteng commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1455674399
This seems to be a breaking change, I would tend to think it's a security enhancement (it's not a vulnerability), wouldn't it be more appropriate to enable it by default in some future version? Now to avoid the impact on user clusters, can we introduce an option to support disable or enable it. @michaeljmarshall cc/@nodece
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1457159677
@tuteng - the proxy is supposed to connect using one of the `proxyRoles` configured in the `broker.conf`. This is somewhat documented here https://pulsar.apache.org/docs/2.11.x/security-authorization/#proxy-roles. If the proxy connects with something other than a proxy role when using TLS authentication, only the proxy's role will be used to verify authorization, and that will likely result in accidental elevation of privileges. In my opinion, this is not a breaking change because it is preventing a state that shouldn't have existed in the first place. However, I guess that conclusion is up for debate.
--
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] nodece commented on a diff in pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#discussion_r1104271313
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java:
##########
@@ -150,19 +150,11 @@ public static boolean isClientAuthenticated(String appId) {
return appId != null;
}
- private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
- String originalPrincipal) {
- if (proxyRoles.contains(authenticatedPrincipal)) {
- // Request has come from a proxy
- if (StringUtils.isBlank(originalPrincipal)) {
- log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
- throw new RestException(Status.UNAUTHORIZED,
- "Original principal cannot be empty if the request is via proxy.");
- }
- if (proxyRoles.contains(originalPrincipal)) {
- log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
- throw new RestException(Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
- }
+ private void validateOriginalPrincipal(String authenticatedPrincipal, String originalPrincipal) {
+ if (!pulsar.getBrokerService().getAuthorizationService()
+ .isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, clientAuthData())) {
+ throw new RestException(Status.UNAUTHORIZED,
Review Comment:
Or `404`?
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1463412653
> in many user environments it can be trusted and there is no such risk
Can you clarify what "it" is here? What is the purpose of authorization if the environment is trusted?
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1423353001
And now I failed with this error:
```
Restoring tar from name pulsar-maven-repository-binaries.tar.zst in GitHub Actions Artifacts to /home/runner
/home/runner/work/_temp/_github_home/.local/bin/gh-actions-artifact-client.js:9924
throw new Error(`Unable to find an artifact with the name: ${name}`)
^
Error: Unable to find an artifact with the name: pulsar-maven-repository-binaries.tar.zst
at ExtendedDownloadHttpClient.downloadStream (/home/runner/work/_temp/_github_home/.local/bin/gh-actions-artifact-client.js:9924:13)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
0.00 B 0:00:00 [0.00 B/s]
zstd: /*stdin*\: unexpected end of file
tar: Child returned status 1
tar: Error is not recoverable: exiting now
Error: Process completed with exit code 2.
```
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1422037686
> I think we should add a paramter in the config file, and when this paramter is enabled, we perform the strict checks.
My primary concern with making this requirement configurable is that it is prone to error. For me, the justification comes here:
https://github.com/apache/pulsar/blob/d7c4e373ac8cb60f234c9c231e5dce5bf7c9b50e/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java#L344-L353
A proxy that forwards an admin call without being configured as a `proxyRole` will only be authorized based on the role supplied by the proxy. Since these `proxyRoles` are often also `superUsers`, this is extremely problematic and easy to misconfigure, especially because everything will "work" when the proxy's auth role is a super user. However, it will work because the proxy is over provisioned and the misconfiguration could lead to elevated permissions by the client.
--
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 a diff in pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#discussion_r1104728175
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -293,19 +292,39 @@ public CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName,
return provider.allowSinkOpsAsync(namespaceName, role, authenticationData);
}
- private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
- String originalPrincipal) {
- if (proxyRoles.contains(authenticatedPrincipal)) {
- // Request has come from a proxy
+ public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
+ String originalPrincipal,
+ AuthenticationDataSource authDataSource) {
+ SocketAddress remoteAddress = authDataSource != null ? authDataSource.getPeerAddress() : null;
+ return isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, remoteAddress);
+ }
+
+ /**
+ * Validates that the authenticatedPrincipal and the originalPrincipal are a valid combination.
+ * Valid combinations fulfill the following rule: the authenticatedPrincipal is in
+ * {@link ServiceConfiguration#getProxyRoles()}, if, and only if, the originalPrincipal is set to a role
+ * that is not also in {@link ServiceConfiguration#getProxyRoles()}.
+ * @return true when roles are a valid combination and false when roles are an invalid combination
+ */
+ public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
+ String originalPrincipal,
+ SocketAddress remoteAddress) {
+ String errorMsg = null;
+ if (conf.getProxyRoles().contains(authenticatedPrincipal)) {
if (StringUtils.isBlank(originalPrincipal)) {
- log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
- throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be empty if the "
- + "request is via proxy.");
- }
- if (proxyRoles.contains(originalPrincipal)) {
- log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
- throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
+ errorMsg = "originalPrincipal must be provided when connecting with a proxy role.";
+ } else if (conf.getProxyRoles().contains(originalPrincipal)) {
+ errorMsg = "originalPrincipal cannot be a proxy role.";
}
+ } else if (StringUtils.isNotBlank(originalPrincipal)) {
+ errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
+ }
Review Comment:
I don't think so. As far as I can tell, there is no requirement for the proxy's role to be a super user. The current authorization logic verifies that the `authenticatedPrincipal` and the `originalPrincipal` both have permission to perform the attempted operation. It would be a change in this paradigm to start requiring the proxy role to be a super user.
--
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 a diff in pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#discussion_r1104784653
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java:
##########
@@ -150,19 +150,11 @@ public static boolean isClientAuthenticated(String appId) {
return appId != null;
}
- private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
- String originalPrincipal) {
- if (proxyRoles.contains(authenticatedPrincipal)) {
- // Request has come from a proxy
- if (StringUtils.isBlank(originalPrincipal)) {
- log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
- throw new RestException(Status.UNAUTHORIZED,
- "Original principal cannot be empty if the request is via proxy.");
- }
- if (proxyRoles.contains(originalPrincipal)) {
- log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
- throw new RestException(Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
- }
+ private void validateOriginalPrincipal(String authenticatedPrincipal, String originalPrincipal) {
+ if (!pulsar.getBrokerService().getAuthorizationService()
+ .isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, clientAuthData())) {
+ throw new RestException(Status.UNAUTHORIZED,
Review Comment:
The current code in `AuthorizationService` throws `Status.UNAUTHORIZED`. Because I don't want to throw a rest exception when `ServerCnx` calls that method, I moved the exception here. I think we should maintain the behavior for which exception is thrown.
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1422037977
I added a commit to expand the scope of this PR so that both admin and binary endpoints have the same requirements.
--
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] nicoloboschi commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1422192933
> > I think we should add a paramter in the config file, and when this paramter is enabled, we perform the strict checks.
>
> My primary concern with making this requirement configurable is that it is prone to error. For me, the justification comes here:
>
> https://github.com/apache/pulsar/blob/d7c4e373ac8cb60f234c9c231e5dce5bf7c9b50e/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java#L344-L353
>
> A proxy that forwards an admin call without being configured as a `proxyRole` will only be authorized based on the role supplied by the proxy. Since these `proxyRoles` are often also `superUsers`, this is extremely problematic and easy to misconfigure, especially because everything will "work" when the proxy's auth role is a super user. However, it will work because the proxy is over provisioned and the misconfiguration could lead to elevated permissions by the client.
@michaeljmarshall good work!
IIUC if the proxy uses a role not included in `proxyRoles` the `originalPrincipal` authorization checks are skipped completely?
so if the proxy uses a super user which is not correctly set in the broker's `proxyRoles`, any client connected through the proxy is considered a superuser by the broker?
--
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] mattisonchao commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1457235310
@michaeljmarshall
How about the `proxy` and `brokerInternalClient` use the same role? It can work before, but it will break by the current. It will reject by `originalRole` can't equal `proxyRole`.
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1464036418
> In a pulsar cluster there are not only super roles, but also non-super roles, so authorization is still required
Sure, and that is why this PR is necessary for the older branches. If the proxy has a super user role that is not in the `proxyRoles` configured by the broker and the proxy is using mTLS to authenticate with the broker, all clients going through the proxy inherit the proxy's superuser role.
> Before this change, if a client wanted to connect to the cluster using the super role, the role had to be configured in the proxy's superUserRoles, otherwise the proxy would not be able to authenticate it, after this change, if a client connected to the cluster using the super role, the role could not appear in superUserRoles of the proxy and proxyRoles of the broker, otherwise the connect also will fail, which seems to be an opposite behavior
This analysis does not match my understanding of the PR. The proxy's `superUserRoles` has nothing to do with this change. This PR only changes the broker's logic to require an `originalPrincipal` to be supplied, the `role` must be one of the `proxyRoles` in the broker.conf. The core logic is here:
https://github.com/apache/pulsar/blob/d4be954dedcc7537b3d65b9a1d7b5662e6062fdf/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java#L332-L335
It sounds like the problem you're encountering is that your proxy's role is not in the `proxyRoles` list. The tests modified by this PR support my understanding of this change.
> I think this is a security enhancement, not a vulnerability (I'm sure you agree)
I disagree that this is only an enhancement. This change protects operators from dangerous misconfigurations.
The only way this is not a vulnerability is if we agree that a proxy is supposed to connect with a role in the `proxyRoles`. It is a vulnerability if we decide that the `proxyRoles` list is irrelevant because it's not the user misconfiguring things but rather pulsar doing the wrong thing.
--
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 #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1423349401
There are some test failures that seem related to incorrect resources getting copied around. I deleted some of the github actions artifacts to try to make it work.
--
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] tuteng commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "tuteng (via GitHub)" <gi...@apache.org>.
tuteng commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1463770662
> > in many user environments it can be trusted and there is no such risk
>
> Can you clarify what "it" is here? What is the purpose of authorization if the environment is trusted?
`it` means is that the client application uses the proxy's super role to connect to the cluster
In a pulsar cluster there are not only super roles, but also non-super roles, so authorization is still required, and if a client has been assigned a super role, it is of course trusted
Before this change, if a client wanted to connect to the cluster using the super role, the role had to be configured in the proxy's superUserRoles, otherwise the proxy would not be able to authenticate it, after this change, if a client connected to the cluster using the super role, the role could not appear in superUserRoles of the proxy and proxyRoles of the broker, otherwise the connect also will fail, which seems to be an opposite behavior
I think this is a security enhancement, not a vulnerability (I'm sure you agree), but now that this change has been introduced on most major branches.I understand it doesn't make sense to use the proxy's superRoles, but it works correctly, and if a user performs a minor version upgrade without understanding the context, such as upgrading a cluster from 2.10.3 to 2.10.4 (ideally there should be no breaking changes), that will result in clients not being able to successfully connect to the cluster, which will cause some failures, which have actually happened in some users' test environments
I don't think it is a problem to introduce this change in a major release (e.g. 3.0) because it will give users enough time to understand it. Minor upgrades are frequent, so I don't think it is appropriate to introduce it on a minor release and not be configurable, which can very easily lead to unpredictable failures
--
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] lhotari commented on pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#issuecomment-1425321271
> @lhotari - any idea on how to get around these failures?
I'll check the reason.
--
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] lhotari merged pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #19455:
URL: https://github.com/apache/pulsar/pull/19455
--
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] codelipenghui commented on a diff in pull request #19455: [improve][broker] Require authRole is proxyRole to set originalPrincipal
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #19455:
URL: https://github.com/apache/pulsar/pull/19455#discussion_r1104208580
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -293,19 +292,39 @@ public CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName,
return provider.allowSinkOpsAsync(namespaceName, role, authenticationData);
}
- private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
- String originalPrincipal) {
- if (proxyRoles.contains(authenticatedPrincipal)) {
- // Request has come from a proxy
+ public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
+ String originalPrincipal,
+ AuthenticationDataSource authDataSource) {
+ SocketAddress remoteAddress = authDataSource != null ? authDataSource.getPeerAddress() : null;
+ return isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, remoteAddress);
+ }
+
+ /**
+ * Validates that the authenticatedPrincipal and the originalPrincipal are a valid combination.
+ * Valid combinations fulfill the following rule: the authenticatedPrincipal is in
+ * {@link ServiceConfiguration#getProxyRoles()}, if, and only if, the originalPrincipal is set to a role
+ * that is not also in {@link ServiceConfiguration#getProxyRoles()}.
+ * @return true when roles are a valid combination and false when roles are an invalid combination
+ */
+ public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
+ String originalPrincipal,
+ SocketAddress remoteAddress) {
+ String errorMsg = null;
+ if (conf.getProxyRoles().contains(authenticatedPrincipal)) {
if (StringUtils.isBlank(originalPrincipal)) {
- log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
- throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be empty if the "
- + "request is via proxy.");
- }
- if (proxyRoles.contains(originalPrincipal)) {
- log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
- throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
+ errorMsg = "originalPrincipal must be provided when connecting with a proxy role.";
+ } else if (conf.getProxyRoles().contains(originalPrincipal)) {
+ errorMsg = "originalPrincipal cannot be a proxy role.";
}
+ } else if (StringUtils.isNotBlank(originalPrincipal)) {
+ errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
+ }
Review Comment:
Do we need to check if `authenticatedPrincipal` is a super user?
--
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