nextgenhealthcare / connect

The swiss army knife of healthcare integration.
Other
931 stars 280 forks source link

[IDEA] Password configuration and security #6098

Open ppazos opened 9 months ago

ppazos commented 9 months ago

Is your feature request related to a problem? Please describe.

SMTP Sender configuration has a password input for using when authentication is Yes. When you write in the input, you can't actually see what you are writing. So it's not possible to use variables in that field.

Screenshot_2024-02-11_20-01-17

I don't think showing asterisks in the password actually serve any security feature, since once you export the channel the password is in the channel XML in plain text :)

<password>thisismypass</password>

If a variable is allowed in the field, I could get an env var, so my channel won't even have a plain text password in it when I export the channel, just a reference for a name of a value I got from env vars.

So current asterisks don't serve any real security feature IMHO and just disables the ability of using configuration variables.

Describe your use case

Se above.

Describe the solution you'd like

  1. Remove the asterisks
  2. Allow variables from configurationMap or other maps
  3. Do not allow plain text passwords in channel's XMLs!!!

Describe alternatives you've considered

Can't find any alternatives.

jonbartels commented 9 months ago

Allow variables from configurationMap or other maps

This already works. It is one of the main reasons that the config map feature was created.

You can enter ${ppazosEmailPassword} and it will read from the variable map lookup sequence and will read the mapped value.

ppazos commented 9 months ago

@jonbartels how can I see what I enter if I can't see what I enter? (the issue is the asterisks)

I actually need to export the channel, open the xml, search for the password and see if that is a variable expression and if it's the correct variable expression, you know typos happen...

pacmano1 commented 9 months ago

You cannot see it. So - the feature request to "reveal password" is a good one, however it needs to be mindful that open source mirth and paid mirth are different with respect to user permissions that can exist in the latter with the user role plugin. So "remove asterisks" is not the fix. It is "provide a way to reveal the password that also works with the user authorization plugin"

jonbartels commented 9 months ago

@ppazos I agree with you 100%. Even if the maps are supported, it is harder to use.

Would a good overall solution be to change the username and password fields to accept a map key as input. That would let the input be unmasked AND force users to use the maps so that credentials are less likely to leak from exports?

ppazos commented 9 months ago

@pacmano1 " however it needs to be mindful that open source mirth and paid mirth are different with respect to user permissions that can exist in the latter with the user role plugin" I don't understand what that means. Please explain the difference and what does the masking actually do for either. Sorry I don't get your point.

@jonbartels IMHO at least for the password which is the only real private/secret data since user could be a public email, Mirth shouldn't allow to record a free text, only variables should be allowed ${xxx}. The reason is simple: if you are a mirth authorized user, you can't see the current password, but you can change it. Though if you want to see the current password you just export the channel and it's there in plain text :)

Then if we share channels publicly, all the passwords will be there in plain text. I do a lot of MC training and actually share channels with my students, sometimes via public places, though I know what I'm doing, people commit errors, like sending a channel to a public support forum, etc. So IMHO all passwords should be externalized, making it the responsibility of the MC user to know and track the passwords set via variables (external configuration from MC or env vars or whatever mechanism).

So masking the password isn't really a solution for anything: if you can see that, you can export the channel and see the password in free text!!!

pacmano1 commented 9 months ago

Mirth Adminisrator without the User Role Plugin = EVERYONE created in "Setting...Users" is an adminstrator.

I called the plugin the wrong thing. It is the "Role-Based Access Control"

With that plugin: there are fine tuned permission you can define for users. For example you might want a read only user that cannnot see any password so just unhiding the password is not a great option

ppazos commented 9 months ago

@pacmano1 I'm only referring to MC open source, if a plugin does something else I don't really care. For instance, if it masks passwords based on permissions/roles, that's OK. So, my points are:

  1. context: MC open source only
  2. masking passwords doesn't add any security, so adding an unmask functionality is just perpetuating that
  3. masking passwords just make it unusable when using variables: you can't see the variable!
  4. it shouldn't allow free text passwords, only variables, that is in fact a more secure approach than just masking and then saving passwords in free text

That would make MC open source a little more secure and usable, at least in terms of the password fields (smtp, sftp, db connectors, etc)