Skip to content
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

Setting metadata value with a secret/variable fails in the end_user_authentication section #26135

Closed
allenhouchins opened this issue Feb 6, 2025 · 12 comments
Assignees
Labels
bug Something isn't working as documented #g-orchestration Orchestration product group :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.

Comments

@allenhouchins
Copy link
Member

Fleet version: 4.63
Web browser and operating system: Any


💥  Actual behavior

I am trying to set a value for metadata via gitops for end_user_authentication using a github secret (ex: $DOGFOOD_SSO_METADATA). This results in the error:Error: failed to unmarshal file error converting YAML to JSON: yaml: line 42: could not find expected ':':

This same secret works without issue under the sso_settings option. I am also able to get the end_user_authentication section to work if I paste the contents of that variable as the value (as a string). This issue seems limited to some validation we are doing specifically in the end_user_authentication section. Setting the values through the UI gives me expected results.

🧑‍💻  Steps to reproduce

  1. Grab the XML Single sign-on metadata out of dogfood
  2. Set it as a variable and pass it to the metadata field under end_user_authentication
  3. Observe error

🕯️ More info (optional)

See this pull request for the error: #26042

@allenhouchins allenhouchins added :incoming New issue in triage process. :reproduce Involves documenting reproduction steps in the issue bug Something isn't working as documented labels Feb 6, 2025
@allenhouchins allenhouchins added :product Product Design department (shows up on 🦢 Drafting board) #g-orchestration Orchestration product group ~released bug This bug was found in a stable release. and removed :reproduce Involves documenting reproduction steps in the issue labels Feb 6, 2025
@rachaelshaw rachaelshaw removed the :incoming New issue in triage process. label Feb 6, 2025
@sharon-fdm sharon-fdm removed their assignment Feb 10, 2025
@sharon-fdm
Copy link
Collaborator

@sgress454
Copy link
Contributor

I wouldn't be surprised if this was due to the contents of the variable, rather than an issue with using a variable for this config.

@sharon-fdm sharon-fdm added :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. and removed :product Product Design department (shows up on 🦢 Drafting board) labels Feb 10, 2025
@sgress454
Copy link
Contributor

@allenhouchins can you DM and let me know how you're setting the env var when testing #26042?

@allenhouchins
Copy link
Member Author

I was going to put this update in #26387 but probably makes more sense to put here with the original issue.


The part that still isn't clear is why this specific multiline variable works in other places in the same yaml file. Have we gotten to the root cause of why this variable works in sso_settings.metadata and not mdm.end_user_authentication.metadata? Are there any QA notes available that show mdm.end_user_authentication.metadata worked and what metadata they used for testing? This is not data that should be publicly visible so it would be good to understand if testing was done using raw string data vs passing this info through a secret and update testing if needed.

The admin/user expectation is that they can copy settings from one place (like the SAML metadata in Google) to another place (like their GitHub secret) and it works without further manipulation since that is what they would be doing in the UI. We also can't have an admin further edit this data before making it a secret because then it won't be compatible with the IdP.

This issue seems to be very specific to mdm.end_user_authentication.metadata as other examples of multiline variables, work without issue too (ex: passing certificate information as a secret into a profile).

CC: @noahtalerman for awareness.

@noahtalerman
Copy link
Member

The admin/user expectation is that they can copy settings from one place (like the SAML metadata in Google) to another place (like their GitHub secret) and it works without further manipulation since that is what they would be doing in the UI

Hey @sgress454 I agree with Allen here.

I don't think we want to ask the user to tweak the metadata they download from Google Workspace. They should be able to download the metadata, paste it into a GitHub env variable (or the UI), and boom they're ready to go.

If that's not working, the fix here is to make that work.

@sgress454
Copy link
Contributor

sgress454 commented Feb 17, 2025

Have we gotten to the root cause of why this variable works in sso_settings.metadata and not mdm.end_user_authentication.metadata?

Yes -- the issue is that the value of the DOGFOOD_END_USER_SSO_METADATA secret in Github has the correct indentation for being placed under sso_settings.metadata in the YAML file, but not for being placed in mdm.end_user_authentication.metadata. So whoever created that variable already followed the guidance which I'm adding in this PR. In order to get mdm.end_user_authentication.metadata to work, we'd need a new secret with the value indented correctly for that spot (likely by just copying it directly from the fleetctl get config output).

The admin/user expectation is that they can copy settings from one place (like the SAML metadata in Google) to another place (like their GitHub secret) and it works without further manipulation since that is what they would be doing in the UI.

I don't agree with the "that is what they would be doing in the UI" part -- if you pasted something from Google into the UI and didn't adjust the indentation to be correct, it would fail. YAML is pretty strict about that, and we don't do any auto-fixing in the UI -- we just complain:
Image

I don't think we want to ask the user to tweak the metadata they download from Google Workspace.

I can't speak to Google Workspace, but whoever set up the DOGFOOD_END_USER_SSO_METADATA secret either already did some tweaking, or happened to copy it from someplace that had the correct indentation (again, my guess would be from fleetctl but I don't know for sure). If we want to try and auto-format these values, I'd argue that's a feature request rather than a bug fix.

@allenhouchins
Copy link
Member Author

if you pasted something from Google into the UI and didn't adjust the indentation to be correct, it would fail.

I think the argument would be if it failed in the UI, then that means we didn't build it correctly to meet the expectation of being able to copy and paste from the IdP into our product. Again, that information can't be manipulated just to be accepted by the UI (or gitops) otherwise the IdP is going to reject it when they receive it.

I can't speak to Google Workspace, but whoever set up the DOGFOOD_END_USER_SSO_METADATA secret either already did some tweaking, or happened to copy it from someplace that had the correct indentation (again, my guess would be from fleetctl but I don't know for sure). If we want to try and auto-format these values, I'd argue that's a feature request rather than a bug fix.

Looking at the value in the UI, which is populated via gitops, you will notice it has all the correct spacing provided by the IdP and is the expected format for SAML metadata xml. Because we aren't auto-formatting these values, the only way it would have gotten this formatting was by copy/pasting it into the secret in this format.

@sgress454
Copy link
Contributor

Looking at the value in the UI, which is populated via gitops, you will notice it has all the correct spacing provided by the IdP and is the expected format for SAML metadata xml. Because we aren't auto-formatting these values, the only way it would have gotten this formatting was by copy/pasting it into the secret in this format.

This is the value we're displaying after having extracted it from the YAML and saving it in the database. If it was originally set via GitOps, it would have been set using correct indentation. It would have had to have been, otherwise it would have been rejected as invalid YAML. I can write an action to output the actual value of the key if we need further proof.

I think the argument would be if it failed in the UI, then that means we didn't build it correctly to meet the expectation of being able to copy and paste from the IdP into our product.

What we have in the UI is a YAML editor, which validates that you have correct YAML. If you paste in a multi-line string that's not correctly formatted, it will let you know. For instance, if I take the metadata from the link above and try to paste it into a multi-line YAML key in the agent options, it will tell me it's invalid until I highlight it and press the tab key until it's in the right spot.

I agree it'd be great to be able to tell users that they can paste a key value into a Github secret and use it as-is anywhere in a GitOps file. I'd like for them not have to do |- either -- just metadata: $MY_METADATA_SECRET. I just think that detecting indentation level based on context and auto-formatting multiline values would be an update to the GitOps feature and/or the UI, and prioritized accordingly. I assume it wasn't in the original requirements for either.

@noahtalerman
Copy link
Member

@allenhouchins: Looking at the value in the UI, which is populated via gitops, you will notice it has all the correct spacing provided by the IdP and is the expected format for SAML metadata xml. Because we aren't auto-formatting these values, the only way it would have gotten this formatting was by copy/pasting it into the secret in this format.

@sgress454: This is the value we're displaying after having extracted it from the YAML and saving it in the database. If it was originally set via GitOps, it would have been set using correct indentation. It would have had to have been, otherwise it would have been rejected as invalid YAML. I can write an action to output the actual value of the key if we need further proof.

@sgress454 gotcha. This value was originally set via GitOps. It's $DOGFOOD_SSO_METADATA.

If I'm understanding correctly, the $DOGFOOD_END_USER_SSO_METADATA and $DOGFOOD_SSO_METADATA values are actually two different values. The former has incorrect spacing/indentation spacing and the latter has correct spacing/indentation.

@sgress454 do you have access to get the GitHub env variables. If yes, can you please paste both of these in a google doc and share it in Fleet's Google drive so we can check them out?

@sgress454
Copy link
Contributor

If I'm understanding correctly, the $DOGFOOD_END_USER_SSO_METADATA and $DOGFOOD_SSO_METADATA values are actually two different values. The former has incorrect spacing/indentation spacing and the latter has correct spacing/indentation.

My guess is that they're the same value, but the indentation is only right for one of them because they're being used in two different parts of the YAML file which are indented differently. I'll see what I can do re: getting the secret values safely.

@allenhouchins
Copy link
Member Author

Worked with @sgress454 to get to the bottom of this (thank you!). The issue was related to how the XML in the variable was unfurling and not having the appropriate tabbing for yaml-compatibility. So it's working as expected but documentation could use some improvements (I'll take this on) and I will also be submitting a Feature Request to be able to figure out how admins can just copy and paste the SAML metadata into a secret without having to properly tab it for unfurling into yaml.

@fleet-release
Copy link
Contributor

Metadata fails, yet
In clouds, a fix brings light,
UI shines bright, right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as documented #g-orchestration Orchestration product group :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.
Projects
None yet
Development

No branches or pull requests

6 participants