Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
thezbyg closed pull request #1198: AMQ-9472 Add test for wildcard producer breaking authorization URL: https://github.com/apache/activemq/pull/1198 -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
cshannon commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2047943899 Bug fix PR opened at https://github.com/apache/activemq/pull/1200 -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
mattrpav commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2047728387 @thezbyg thank you for the detailed report and staying with it while we work through your scenario to find the bug. A tricky edge case! -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
cshannon commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2047625950 @thezbyg - That is interesting, I do not see that happen when testing some of the Java STOMP tests we have but it could depend on the client. Regardless, that processConsumerControl() method has a bug and should be fixed I think and applies to really any protocol that might send it (even OpenWire if updating prefetch). The purpose of that lookup is to wake up an existing destination for dispatching. So in this case really we just want to be looking up and not creating. This is an edge case because normally the consumer control would not be sent if the consumer hadn't already subscribed and created the destination, so obviously wildcards break this if not already created. For a fix I think there's 2 options. One is to simply never create a destination in that method, just look up existing but I wonder if that could expose unintended bugs. Option 2 is probably safer and that is just to apply similar logic that exists in the `addConsumer() ` method where if it's a normal destination keep the current behavior, but if it's a pattern/wildcard we just do a lookup only and skip if not found. I can work on a PR for option 2, should be a simple fix and I think makes the most sense. I will likely open up a new JIRA since this isn't related to authorization or producing as the original Jira describes. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
thezbyg commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2046523027 > So this is just how the broker currently works to handle wildcard subscriptions, since you are publishing to a wildcard topic then any subscription that matches needs to also subscribe tot he wildcard to get that message. Thanks for the explanation. Everything makes sense, except that auto-creation of wildcard topic can also be triggered by a wildcard consumer and results in the same issue. This happens when subscribing to wildcard topic by using STOMP protocol. In the broker code I can see that wildcard topic is not auto-created for wildcard or composite consumer destination: https://github.com/apache/activemq/blob/e025e443e65d4bd3c2c27f11d6caa7bfbd2c9626/activemq-broker/src/main/java/org/apache/activemq/broker/region/AbstractRegion.java#L344 No such condition exists in processConsumerControl() method: https://github.com/apache/activemq/blob/e025e443e65d4bd3c2c27f11d6caa7bfbd2c9626/activemq-broker/src/main/java/org/apache/activemq/broker/region/AbstractRegion.java#L694 When using STOMP protocol, processConsumerControl() is called immediately after subscribe and auto-creates the wildcard topic. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
mattrpav commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2045239359 Looking at the use case, I don't see a scenario where using a wildcard _topic_ makes the most sense. Topics already of subscriptions, so instead of creating additional _destinations_ for the consumers of the message flow, the apps should create a _subscription_ to a fully qualified non-wildcard topic. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
mattrpav commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2043874595 I think that the behavior should match the destination policy. 1. Wildcard 2. Fully qualified name (authoritative) I agree, it should be a new flag since we would not want to change behavior on existing users. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
cshannon commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2043856614 The only other thing i can think of is we could technically change the behavior of the authorization broker plugin because you could make an argument that any matching wildcard should implicitly be authorized if it matches a topic. In this case by publishing to a wildcard the intent is anything that matches gets it so should be authorized. However, the problem with changing that would be a change in the current authorization behavior which is not good for existing users not expecting it. So I think that we'd need to have a config option or an extension to prevent unintended consequences since it's been like this for a long time. So I would view any changes here more as an enhancement or improvement. If we made it configurable I think it would need to be something where the config would be described as "implicitly grant matching wildcard destinations" or something like that and probably a new feature and not a bug fix release. Subscribing to a wildcard destination would also need to be explored 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
cshannon commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2043790765 In terms of working around this, the options are pretty much: 1. If you are going to create topics then you need to make sure you have proper ACLs set up for those topics (including matching subscriptions on wildcards) that consumers might subscribe to. In this case you could add read ACLs for A.> for the users group. 2. You could always customize the authorization logic by implementing your own plugin or overriding/extending the `AuthorizationBroker` and `AuthorizationDestinationInterceptor` -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
cshannon commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2043752900 This is not really a bug to me, it's a side effect of how wild card subscriptions work in the broker. The issue here is not with the authorization plugin or logic but with how subscriptions currently work. When consumers are created, subscriptions get added for all matching destinations. In this case, there are 2 matching destinations because of the wildcard destination that was auto created. When creating a consumer on Topic A.B, first the new consumer is [authorized](https://github.com/apache/activemq/blob/e025e443e65d4bd3c2c27f11d6caa7bfbd2c9626/activemq-broker/src/main/java/org/apache/activemq/security/AuthorizationBroker.java#L148) on that destination and that passes. But then later on the addSubscription() is called for both destinations to add two subscriptions for the new consumer...one on A.B which is properly [authorized](https://github.com/apache/activemq/blob/e025e443e65d4bd3c2c27f11d6caa7bfbd2c9626/activemq-broker/src/main/java/org/apache/activemq/security/AuthorizationDestinationFilter.java#L40) and then another on A.> which is not authorized because there's no proper ACLs for "users" when adding the subscription for A.> So this is just how the broker currently works to handle wildcard subscriptions, since you are publishing to a wildcard topic then any subscription that matches needs to also subscribe tot he wildcard to get that message. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AMQ-9472 Add test for wildcard producer breaking authorization [activemq]
mattrpav commented on PR #1198: URL: https://github.com/apache/activemq/pull/1198#issuecomment-2042861290 I was able to reproduce using a stock Apache v6.1.1 build. This appears to be a 'does not permit' issue and not a 'does not restrict' issue. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
