Fixes #37418 - Fixes an issue that caused hidden Ansible variables to be shown in plain text on the Host-Details page #717

Thorben-D · 2024-05-08T13:15:45Z

Variables marked as hidden were shown in plain text under Variables and Inventory on a host's details page.
This PR fixes that by masking the values in question in the UI.
Values are still shown in plain text when editing, as this requires the same permissions, edit_ansible_variables, as

Configure > Ansible > Variables.

It should be noted, that hidden variables are NOT considered secrets. The point of hidden is to only hide the values of the respective variables in the UI. The Foreman documentation clearly reflects this fact under point 6.

Changes:

Add "hiddenValue" to GraphQL query hostVariableOverrides.gql
Replace plain text secret with masked value
Add a parameter "redact_secrets" to AnsibleInventoriesController#show_inventory
Change frontend code to use newly added "redact_secrets" parameter
Add a new "to_hash_with_secrets_redacted" method to InventoryCreator

Requires #716

…rrect for hidden_value and override

nofaralfasi · 2024-05-20T12:07:44Z

I'm getting the following error when navigating to the Variables tab:

RuntimeError (Failed to implement AnsibleVariable.hiddenValue, tried:
- `Types::OverridenAnsibleVariable#hidden_value`, which did not exist
- `Presenters::OverridenAnsibleVariablePresenter#hidden_value`, which did not exist
- Looking up hash key `:hidden_value` or `"hidden_value"` on `#<Presenters::OverridenAnsibleVariablePresenter:0x00007f28c28ced48>`, but it wasn't a Hash
To implement this field, define one of the methods above (and check for typos)

Thorben-D · 2024-05-21T07:14:35Z

@nofaralfasi I think that is because you still have the broken GQL scheme... Did you make sure the content of #716 is present on your branch?

nofaralfasi · 2024-05-21T09:50:18Z

@nofaralfasi I think that is because you still have the broken GQL scheme... Did you make sure the content of #716 is present on your branch?

You are right, I missed that part.
Now the Variables tab is shown properly. However, the default value of the hidden variable is still visible through the GraphQL response.
It's a little more complicated to find it, but still very possible.

Also, it's not possible to edit the variable value from the Variables tab. I see it's not related to the changes here, but we need to take care of it as well.

Thorben-D · 2024-05-21T10:34:09Z

Glad you got it sorted.
Yes, the value may indeed still be extracted from the query response. I suppose we could check if the user has the edit_ansible_variables permission and require it for the value to be queried.

I tried to reproduce the issue you faced with editing the value, but without success.
The default value is not changed by this action, but the value for this particular host is. For me, this is then shown under:
Configure > Ansible > Variables > Variable > Specify Matchers

nofaralfasi · 2024-05-21T14:14:49Z

Glad you got it sorted. Yes, the value may indeed still be extracted from the query response. I suppose we could check if the user has the edit_ansible_variables permission and require it for the value to be queried.

Exactly. That should be the correct implementation.

I tried to reproduce the issue you faced with editing the value, but without success. The default value is not changed by this action, but the value for this particular host is. For me, this is then shown under: Configure > Ansible > Variables > Variable > Specify Matchers

I apologize for the confusion, it was a problem on my setup. I'll be more careful next time.

Thorben-D · 2024-05-21T15:17:03Z

Great, I'll implement that then!
No worries about the mix-up, thanks for having a look!

app/graphql/presenters/overriden_ansible_variable_presenter.rb

app/services/foreman_ansible/inventory_creator.rb

Thorben-D · 2024-07-02T07:38:23Z

What is the status on this? I think the code looks pretty good now.

nofaralfasi

Currently, any user with Viewer permissions can see the hidden values of the Ansible variables.
The correct behavior should be as follows: if a user is not an admin and does not have the edit_ansible_variables permission, they should not be able to see the hidden values of the Ansible variables.

… be shown in plain text on the Host-Details page - Add "hiddenValue" to GraphQL query hostVariableOverrides.gql - Replace plain text secret with masked value - Adds a parameter "redact_secrets" to AnsibleInventoriesController#show_inventory - Change frontend code to use newly added "redact_secrets" parameter - Add a new "to_hash_with_secrets_redacted" method to InventoryCreator - Hide hidden values in GQL response by if edit_ansible_variables not granted

…ory and api/v2/ansible_variables if a user does not posess the "edit_ansible_roles" permission

Thorben-D · 2024-07-09T14:33:16Z

Indeed, I only hid the values for the overrides. Obviously the same needed to be done for the default value.
I also added a requirement for the "edit_ansible_variables" permission to view the value of hidden Ansible variables via the API and the Ansible inventory.
I am very unsure whether this is a good idea though, so I added those as a second commit for easy dropping. Also, I am sure that the filtering for the API call can be done much cleaner with ActiveSupport::ParameterFilter, I am just not sure how...

nixfu · 2024-07-10T17:50:32Z

FYI, I think this same issue is affecting the ansible inventory report in Foreman that is used for ansible/AWX to use Foreman as an inventory source. Previously any hidden params in Foreman, would show up as **** in the hostvars of the inventory json. Now they are being shown as their full value and no longer hidden.

nofaralfasi · 2024-07-11T10:03:45Z

FYI, I think this same issue is affecting the ansible inventory report in Foreman that is used for ansible/AWX to use Foreman as an inventory source. Previously any hidden params in Foreman, would show up as **** in the hostvars of the inventory json. Now they are being shown as their full value and no longer hidden.

I've also noticed this behavior. However, could you clarify how it's related to the changes in the PR? The results haven't changed with this PR. It seems we may need to open a separate case to report this issue, unless I'm missing something.

nofaralfasi

To summarize:

I believe we don’t need the new to_hash_with_secrets_redacted method or the redact_secrets parameter. Checking the user’s permissions directly on ForemanAnsible::AnsibleInfo#ansible_params should be sufficient.
We should simplify the GQL query by removing unnecessary fields, such as override and hidden_value.
Most of the functionality already exists; it's better to utilize it where possible. For example, use variable.meta.canEdit instead of variable.hiddenValue.
Preserve existing functionality: If authorized users were previously able to view hidden Ansible variable values in the Host inventory, this access should remain unchanged.

app/graphql/presenters/overriden_ansible_variable_presenter.rb

nofaralfasi · 2024-09-02T14:59:15Z

app/graphql/presenters/overriden_ansible_variable_presenter.rb

+    def override
+      ansible_variable.override?
+    end


Where are we using this value?

Currently nowhere. This is added by #716
You mentioned removing unnecessary fields. This one could be removed, but it will have to be done in the other PR.

app/graphql/presenters/overriden_ansible_variable_presenter.rb

nofaralfasi · 2024-09-02T15:12:37Z

app/graphql/presenters/overriden_ansible_variable_presenter.rb

-      @override_resolver.resolve @ansible_variable
+      resolved = @override_resolver.resolve @ansible_variable
+      unless resolved.nil?
+        resolved[:value] = '*****' unless ansible_variable.editable_by_user?


Same here, we can use ansible_variable.hidden_value instead of the asterisks.

nofaralfasi · 2024-09-02T15:42:40Z

webpack/components/AnsibleHostDetail/components/AnsibleHostInventory/index.js

@@ -9,7 +9,10 @@ import AnsibleHostInventory from './AnsibleHostInventory';
 import ErrorState from '../../../ErrorState';

 const WrappedAnsibleHostInventory = ({ hostId }) => {
-  const params = useMemo(() => ({ params: { host_ids: [hostId] } }), [hostId]);
+  const params = useMemo(
+    () => ({ params: { host_ids: [hostId], redact_secrets: true } }),


Why are we always sending redact_secrets: true? Shouldn't we allow users with the appropriate permissions to view the hidden values?

For the inventory, I am unsure what's best. As opposed to the variables, there is no action to toggle the visibility of hidden variables. I chose to hide hidden variables in the inventory for everyone, as permitted users are able to view those values under the "Variables" tab. I also feel like showing the values by default sort of defeats the purpose of hiding them.
Though, I can change this if you disagree.

As I mentioned, I prefer not to change the current behavior where authorized users can view hidden values in the inventory. Consider a scenario where someone is using Foreman with a script that makes API calls to retrieve the inventory and relies on those variable values. If we change this behavior, it could break their setup. However, if you believe your approach is better, feel free to initiate a discussion in the community to gather more opinions.

I should note that the variables are only hidden if redact_secrets=true is passed during the API call. That is also the reason I have to pass this value through a couple of functions. This only happens in the UI.

nofaralfasi · 2024-09-02T15:51:15Z

app/services/foreman_ansible/inventory_creator.rb

+      to_hash_with_secrets_redacted(false)
+    end
+
+    def to_hash_with_secrets_redacted(redact_secrets = true)


I don't see a need to add this new method if it’s just duplicating the content of to_hash. It seems to complicate things unnecessarily. I suggest simply adding the redact_secrets parameter to the original method and making the necessary changes there.

nofaralfasi · 2024-09-02T16:17:13Z

app/services/foreman_ansible/ansible_info.rb

    end

-    def ansible_params
+    def ansible_params(redact_secrets = false)


If the value of redact_secrets is based on the user's permissions, why do we need to pass redact_secrets to this method? Can't we check if the user has the necessary permissions directly within the method instead?

nofaralfasi · 2024-09-02T16:24:40Z

...AnsibleHostDetail/components/AnsibleVariableOverrides/AnsibleVariableOverridesTableHelper.js

@@ -22,6 +23,21 @@ export const formatValue = variable => {
    ? variable.currentValue.value
    : variable.defaultValue;

+  if (variable.hiddenValue) {


Is the hiddenValue necessary here, or could we rely on variable.meta.canEdit instead?

Unfortunately, the hiddenValue field is needed to decide whether the "fake" variable should be shown instead of the real one. Relying on canEdit here would not work, as a permitted user can edit hidden and "not-hidden" variables.

On second thought, this block isn't necessary. If the user is authorized to view hidden values, we can simply display them as is. If they aren’t authorized, the value should already be hidden by the backend before it's sent.

Fixes #37417 - OverriddenAnsibleVariablePresenter GraphQL scheme inco…

1b2ed8b

…rrect for hidden_value and override

pr-processor bot added the Not yet reviewed label May 8, 2024

ares mentioned this pull request May 9, 2024

Fixes #36595 - Don't show the value for hidden ansible variables #685

Closed

nofaralfasi self-assigned this May 20, 2024

Thorben-D force-pushed the OR-4732_hidden_ansible_variable_not_hidden_in_host_details branch from d468a18 to 0792fb6 Compare May 22, 2024 09:21

nofaralfasi reviewed Jun 5, 2024

View reviewed changes

app/graphql/presenters/overriden_ansible_variable_presenter.rb Outdated Show resolved Hide resolved

nofaralfasi reviewed Jun 5, 2024

View reviewed changes

app/services/foreman_ansible/inventory_creator.rb Show resolved Hide resolved

Thorben-D force-pushed the OR-4732_hidden_ansible_variable_not_hidden_in_host_details branch from 0792fb6 to bea2b37 Compare June 7, 2024 07:28

Thorben-D force-pushed the OR-4732_hidden_ansible_variable_not_hidden_in_host_details branch from bea2b37 to d32fbb2 Compare June 18, 2024 09:28

nofaralfasi requested changes Jul 7, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Jul 7, 2024

hosekadam and others added 4 commits July 9, 2024 15:13

Add the env var

00c8843

Fixes #37404 - Drop webpacked_plugins_css_for from code

0db5002

EXP: Prevent hidden variables from being shown through ansible invent…

cfb1bc0

…ory and api/v2/ansible_variables if a user does not posess the "edit_ansible_roles" permission

Thorben-D force-pushed the OR-4732_hidden_ansible_variable_not_hidden_in_host_details branch from d32fbb2 to cfb1bc0 Compare July 9, 2024 14:28

pr-processor bot added the Needs re-review label Jul 9, 2024

Thorben-D mentioned this pull request Aug 28, 2024

Fixes #37417 - OverriddenAnsibleVariablePresenter GraphQL scheme incorrect for hidden_value and override #716

Open

nofaralfasi requested changes Sep 2, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37418 - Fixes an issue that caused hidden Ansible variables to be shown in plain text on the Host-Details page #717

Fixes #37418 - Fixes an issue that caused hidden Ansible variables to be shown in plain text on the Host-Details page #717

Thorben-D commented May 8, 2024

nofaralfasi commented May 20, 2024

Thorben-D commented May 21, 2024

nofaralfasi commented May 21, 2024

Thorben-D commented May 21, 2024 •

edited

Loading

nofaralfasi commented May 21, 2024

Thorben-D commented May 21, 2024

Thorben-D commented Jul 2, 2024

nofaralfasi left a comment

Thorben-D commented Jul 9, 2024

nixfu commented Jul 10, 2024

nofaralfasi commented Jul 11, 2024

nofaralfasi left a comment •

edited

Loading

nofaralfasi Sep 2, 2024

Thorben-D Sep 3, 2024

nofaralfasi Sep 2, 2024

nofaralfasi Sep 2, 2024

Thorben-D Sep 3, 2024

nofaralfasi Sep 3, 2024

Thorben-D Sep 3, 2024

nofaralfasi Sep 2, 2024

nofaralfasi Sep 2, 2024

nofaralfasi Sep 2, 2024

Thorben-D Sep 3, 2024

nofaralfasi Sep 3, 2024

Fixes #37418 - Fixes an issue that caused hidden Ansible variables to be shown in plain text on the Host-Details page #717

Are you sure you want to change the base?

Fixes #37418 - Fixes an issue that caused hidden Ansible variables to be shown in plain text on the Host-Details page #717

Conversation

Thorben-D commented May 8, 2024

nofaralfasi commented May 20, 2024

Thorben-D commented May 21, 2024

nofaralfasi commented May 21, 2024

Thorben-D commented May 21, 2024 • edited Loading

nofaralfasi commented May 21, 2024

Thorben-D commented May 21, 2024

Thorben-D commented Jul 2, 2024

nofaralfasi left a comment

Choose a reason for hiding this comment

Thorben-D commented Jul 9, 2024

nixfu commented Jul 10, 2024

nofaralfasi commented Jul 11, 2024

nofaralfasi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thorben-D commented May 21, 2024 •

edited

Loading

nofaralfasi left a comment •

edited

Loading