-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Run activemq container as non root #1378
base: main
Are you sure you want to change the base?
Conversation
|
Hi @mattrpav thanks for taking the time to review.
Taking a look at the 5.x distribution I saw the default user was using jetty-realm properties file which was only available in 5.x dists so I chose to rely on the presence of this file to decide wether I would set the default web user |
@mattrpav as you renamed this PR title I was wondering whether you were expecting more work from me or are yo planning to take over from here? |
6a098d1
to
4d15090
Compare
I'm sorry, but I don't understand the overall purpose. What's exactly the problem statement ? |
Sorry if that's not clear. What I'm trying to do here is to produce an image which will:
That's mainly to conform to 2 container best practices: to not run as root and to not run container with a read-only fs |
Ok, it's clearer for me now. As I see more than "just" non root privileges, it wasn't obvious to me. Let me do a new review/pass (fyi, I'm the original author of the Docker image build). |
-s '/d:beans/a:broker' -t elem -n plugins \ | ||
-s '/d:beans/a:broker/plugins' -t elem -n simpleAuthenticationPlugin \ | ||
-s '/d:beans/a:broker/plugins/simpleAuthenticationPlugin' -t elem -n users \ | ||
-a '/d:beans/a:broker/plugins/simpleAuthenticationPlugin' -t attr -name anonymousAccessAllowed -v true \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling anonymous access should not be the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I first tried to make it point to another env var but it did not work and I concluded the plugin attribute anonymousAccessAllowed
is simply not looked up from the java system properties unlike authenticationUser
attribute (e.g. username
or password
).
Given the default behavior of the exisitng image was already to come up with anonymous access enabled (one has to provide $ACTIVEMQ_CONNECTION_USER
to disable it), I've decided to let anonymous access enabled b default as well.
I personally have no problem with setting it to false b default instead, unless there's a way to make the plugin initialization lookup a java system property for anonymousAccessAllowed
?
I'm wondering if there is a better approach where we can have a simple generic config by default, and a config flag to replace ${activemq.conf} location. This would allow user to provide a volume with any set of configuration files the wish. |
@mattrpav yeah. I'm not a big fan deviating from the default amq config. |
That's definitely a good approach for advanced config where having env vars or doing on-the-fly modification of config files will never really be enough. However for the most simple cases where one may just want to set credentials having to create a volume (which may vary greatly whether you're running just docker or kubernetes or or something else) and having to scaffold it with he right version of the config, may look a bit too much |
Hi gentlemen, Reaching out to know whether you came to a conclusion on that PR? Should I change the current behaviour and make the image reject anonymous request by default? Or do you prefer to simply discard the PR and come up with something of your own? |
@alxgomz I'm doing a new pass (I'm preparing the next ActiveMQ releases). |
This PR is a proposal to address https://issues.apache.org/jira/browse/AMQ-9588
It's trying to keep the same behavior of the container and essentially:
anonymousAccessAllowed
param ofsimpleAuthenticationPlugin
so I left it on by default to mimic to previous behavior)