ARTEMIS-1600 Support masked passwords in bootstrap.xm and login.config#1771
ARTEMIS-1600 Support masked passwords in bootstrap.xm and login.config#1771gaohoward wants to merge 1 commit into
Conversation
|
Aside from the failing tests this looks OK. That said, I would love to see us move all our password masking to use the "ENC()" syntax instead of using boolean "mask-password" attributes everywhere if possible. This is done for the properties login module and it's very clean. If the text follows the pattern then it should be treated as masked otherwise it shouldn't. |
|
@jbertram good point. I think I can do that. Do you think we still need to support "mask-password" for backward compatibility? |
|
@jbertram ok I think we can support ENC() as well as "mask-password". |
|
We should keep mask-password config support where it exists already, but don't add any new features that use it. Instead we can rely on the ENC() syntax. |
|
OK, so I'd add ENC() syntax and keeps the mask-password as an option. (I mean I won't remove this from this PR, but won't add any more in the future if there is new password mask requirements). |
|
In my opinion, you should remove the mask-password config property from this PR as it will require more code/documentation changes later when it's deprecated and eventually removed. |
|
that's fine. I'll remove it. |
gaohoward
force-pushed
the
fent908
branch
2 times, most recently
from
eec4197 to
4c94dbf
Compare
|
@jbertram Hi Justin, I think it's done. Can you take a look again? Thanks |
|
well, almost done. Just found a unused var. I'll delete it right away. Sorry about that. :) |
gaohoward
force-pushed
the
fent908
branch
2 times, most recently
from
568e701 to
41b7883
Compare
| */ | ||
| public static boolean isDefaultMaskPassword() { | ||
| return DEFAULT_MASK_PASSWORD; | ||
| public static Boolean isDefaultMaskPassword() { |
There was a problem hiding this comment.
Why this?
Why you need a third state? true / false / undefined?
Why not just keep it boolean.. either mask it or not...
Besides.. if you really need the third option.. I would make DEFAULT_MASK_PASSWORD = null; instead of returning null here.
I couldn't understand why you need it.
There was a problem hiding this comment.
@clebertsuconic, check out org.apache.activemq.artemis.utils.PasswordMaskingUtil.resolveMask. I believe the way that method works is why he needs 3 values.
There was a problem hiding this comment.
@clebertsuconic yes that's the purpose as @jbertram pointed out. I need 'null' to represent the case where the mask-password is not specified at all.
| } | ||
| isRoleAttributeSet = isLoginPropertySet(ROLE_NAME); | ||
| roleAttributeName = getLDAPPropertyValue(ROLE_NAME); | ||
| String isMask = (String) options.get(MASK_PASSWORD); |
There was a problem hiding this comment.
This isn't used anywhere so it can be removed.
| passwords. | ||
|
|
||
| In general, a masked password can be identified using one of two ways. The first one | ||
| iS the ENC() syntax, i.e. any string value wrapped in ENC() is to be treated as |
There was a problem hiding this comment.
Capitalization error on "iS".
gaohoward
force-pushed
the
fent908
branch
2 times, most recently
from
9647a36 to
1e04cef
Compare
|
@jbertram @clebertsuconic done! |
|
@jbertram Please hold for a moment. I just found a change that may be wrong. |
We provide a feature to mask passwords in the configuration files. However, passwords in the bootstrap.xml (when the console is secured with HTTPS) cannot be masked. This enhancement has been opened to allow passwords in the bootstrap.xml to be masked using the built-in masking feature provided by the broker. Also the LDAPLoginModule configuration (in login.config) has a connection password attribute that also needs this mask support. In addition the ENC() syntax is supported for password masking to replace the old 'mask-password' flag.
|
OK it's done. |
4 participants
We provide a feature to mask passwords in the configuration files.
However, passwords in the bootstrap.xml (when the console is
secured with HTTPS) cannot be masked. This enhancement has
been opened to allow passwords in the bootstrap.xml to be masked
using the built-in masking feature provided by the broker.
Also the LDAPLoginModule configuration (in login.config) has a
connection password attribute that also needs this mask support.