fleet icon indicating copy to clipboard operation
fleet copied to clipboard

GitOps: Fleet doesn't escape characters not supported in XML when resolving variables

Open reeblybeebly opened this issue 3 months ago • 13 comments

Fleet version: 4.72.0


💥  Actual behavior

customer-reedtimmer stated they were experiencing and issue with gitops bulk uploading macOS Configuration Profiles. One profiles is causing an issue. The process fails with the problem profile attached to a team, reporting the following error...

POST ******/api/latest/fleet/mdm/profiles/batch?assume_enabled=false&overwrite=true&team_name=Pilot 502 Bad Gateway (78ms)
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
</body>
</html>
Error: applying custom settings for team "Pilot": POST /api/latest/fleet/mdm/profiles/batch received status 502 unknown

customer-reedtimmer provided configuration crofile

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1">
  <dict>
    <key>PayloadUUID</key>
    <string>DFC8CEC8-49DE-4FF6-874F-8B613F1CA4A9</string>
    <key>PayloadType</key>
    <string>Configuration</string>
    <key>PayloadOrganization</key>
    <string><REDACTED, inc.</string>
    <key>PayloadIdentifier</key>
    <string>DFC8CEC8-49DE-4FF6-874F-8B613F1CA4A9</string>
    <key>PayloadDisplayName</key>
    <string>Network: network name - REDACTED</string>
    <key>PayloadDescription</key>
    <string>Joins the REDACTED SSID for our REDACTED offices</string>
    <key>PayloadVersion</key>
    <integer>1</integer>
    <key>PayloadEnabled</key>
    <true/>
    <key>PayloadRemovalDisallowed</key>
    <true/>
    <key>PayloadScope</key>
    <string>System</string>
    <key>PayloadContent</key>
    <array>
      <dict>
        <key>PayloadUUID</key>
        <string>DBD98BFF-120A-45D4-B741-81EE79B2923E</string>
        <key>PayloadType</key>
        <string>com.apple.wifi.managed</string>
        <key>PayloadOrganization</key>
        <string>REDACTED, Inc.</string>
        <key>PayloadIdentifier</key>
        <string>DBD98BFF-120A-45D4-B741-81EE79B2923E</string>
        <key>PayloadDisplayName</key>
        <string>WiFi REDACTED</string>
        <key>PayloadDescription</key>
        <string/>
        <key>PayloadVersion</key>
        <integer>1</integer>
        <key>PayloadEnabled</key>
        <true/>
        <key>HIDDEN_NETWORK</key>
        <false/>
        <key>Password</key>
        <string>$FLEET_SECRET_REDACTED</string>
        <key>EncryptionType</key>
        <string>WPA</string>
        <key>AutoJoin</key>
        <true/>
        <key>CaptiveBypass</key>
        <false/>
        <key>ProxyType</key>
        <string>None</string>
        <key>SetupModes</key>
        <array/>
        <key>SSID_STR</key>
        <string>REDACTED</string>
        <key>Interface</key>
        <string>BuiltInWireless</string>
      </dict>
    </array>
  </dict>
</plist>

🛠️ To fix

Fleet already escapes special characters before sending the profile to a host.

Fleet should escape values before sending them to the host, for following variable types:

  • Fleet custom variables (aka secrets FLEET_SCERET_...)
  • GitHub/GitLab variables
  • Fleet's dynamic variables that are listed here (FLEET_VAR_...).

Fleet should only escape variables if they are used in .mobileconfig profiles.

If the user tries to add FLEET_SECRET_ variable with XML in it, or adds XML to a GitHub/GitLab variable and runs GitOps, Fleet should throw an error.

Couldn't add a custom variable. Value can't include XML.

GitOps:

1 error occured: Couldn't edit "workstations.yaml" at "controls.macos_settings_custom_settings": variable can't include XML.

Docs PR: #34576

Product designer: @marko-lisica

🧑‍💻  Steps to reproduce

  1. Add Fleet secret that includes characters not supported in XML (&, <, >, ", ')
  2. Use a secret in the configuration profile
  3. Run GitOps and observe error 503: unknown

🕯️ More info (optional)

Conversatiuon with customer: https://fleetdm.slack.com/archives/C052K2LAMCP/p1757077212308309

reeblybeebly avatar Sep 12 '25 14:09 reeblybeebly

I'm unable to reproduce when running fleetctl gitops via CLI but I will attempt again with a GitHub Repo using Actions when able.

PezHub avatar Sep 17 '25 05:09 PezHub

I was finally able to get back to this and tested with a GitOps repo - After adding a secret to the env section of my workflow.yml file I'm able to repro the issue. It's not the same 502 bad gateway error the customer sees but instead Error: applying custom settings for team "💻 Workstations": POST /api/latest/fleet/mdm/profiles/batch received status 503 unknown

Instead, we should throw an error about the unsupported character like we do when running GitOps via CLI - e.g.

~/fleetdm/fleet main *23 +1 !1 ?1 ❯ export FLEET_SECRET_PASSWORD='test&123'                                                                                      06:01:11 PM
~/fleetdm/fleet main *23 +1 !1 ?1 ❯ ./build/fleetctl gitops -f ~/fleetdm/gitops-local/PezHub/teams/QAGitops.yml                                                  06:02:17 PM
Warning: Version mismatch.
Client Version:   orbit-v1.45.0-864-g649ed67760
Server Version:  orbit-v1.45.0-864-g649ed67760-dirty
Error: Team QA GitOps: Couldn't edit macos_settings.custom_settings (secret_variables_disableguest.mobileconfig): plist: error parsing XML property list: XML syntax error on line 17: invalid character entity &123 (no semicolon)

cc: @georgekarrv

PezHub avatar Sep 26 '25 21:09 PezHub

Hey @JordanMontgomery, when you get the chance, can you take a look at "To fix" section.

I believe we should be able to escape Fleet secrets before deploying the profile. What about GitHub's variables and secrets? Can we escape those? I assume we might need to do that in fleetctl?

Also, do you think this should go to #g-orchestration as they own GitOps now?

marko-lisica avatar Oct 02 '25 12:10 marko-lisica

I actually think this one might be on #g-mdm I am able to repro what gabe sees by putting test&123 into a variable and replacing it. We might need to reconsider how we support variables since currently we allow them in various parts of config profiles and in some places they should be XML escaped as a string value and other places they should not be(for instance they could contain raw XML)

Image

JordanMontgomery avatar Oct 02 '25 13:10 JordanMontgomery

@JordanMontgomery Can we detect if a variable is used inside XML vs. if it is XML? If so, I think then we want to escape in first case. I'll specify that as a solution, and if you think it's not feasible, please let me know.

marko-lisica avatar Oct 07 '25 18:10 marko-lisica

@marko-lisica it may be feasible in some cases but not in all cases. And fleet may not be able to detect if a variable is used within a portion of the XML we need to escape or not because it could be invalid XML we cannot parse until the secret is substituted.

JordanMontgomery avatar Oct 07 '25 19:10 JordanMontgomery

@JordanMontgomery I think we can be opinionated here. Having XML as a variable isn't something we think users should do. I think we should document that Fleet will escape all variables when they are used in XML (.mobileconfig and Windows profiles). No matter what they provide, we escape special characters for Mac and Windows profiles. Wdyt?

marko-lisica avatar Oct 08 '25 11:10 marko-lisica

@JordanMontgomery I just specified "To fix" for this bug + some docs changes. Wdyt?

marko-lisica avatar Oct 21 '25 11:10 marko-lisica

@marko-lisica I think we should also decide what the behavior will be for things other than XML profiles, like JSON DDM and scripts and what to do for each platform. Also to make the changes clearer to users. For this ticket the root of the problem is that fleet secret variables(i.e. FLEET_SECRET_...) don't get escaped at all when being substituted into documents. This can be verified in the code by looking at the ExpandEmbeddedSecrets datastore method. I don't think this is a problem for built-in Fleet variables as Fleet limits where they can be placed, and their values, but since fleet secrets are more or less arbitrary there are some potential problems.

I don't think we can completely block XML within fleet secrets because a user could conceivably have a valid secret like a password which looks like XML but isn't. Since we also support these other document types(.sh scripts, powershell scripts and JSON) I think this is bigger than just an XML problem and the solution should likewise be.

JordanMontgomery avatar Oct 21 '25 14:10 JordanMontgomery

@JordanMontgomery I think for JSON profiles we don't need to escape, so this would apply only for XML based profiles - .mobileconfig and Windows XML (which we don't support yet).

Re: Fleet built-in variables: For example, $FLEET_VAR_HOST_END_USER_IDP_GROUPS might include group names that have special characters, which won't work in the profile. Therefore, I believe we should escape built-in variables as well.

Can we check if variable content is valid XML? I think we do that check for profiles.

marko-lisica avatar Oct 22 '25 10:10 marko-lisica

@marko-lisica That is correct for those variables we escape the values when substituting them in and sending profiles. So I don't think we need any changes for builtin variables. It's just fleet secret variables that aren't escaped.

I don't see anywhere that we actually check if the variable content is valid XML. we substitute the variable contents in during validation and make sure that the profile after the variable is substituted is valid XML but that's it

Perhaps a simpler more targeted fix for this is that fleet, when it is sending or validating XML profiles, also XML escapes fleet secrets just like we do for builtin variables. If we narrow it down to that it should fix the customer issue and minimize side effects. There is still the potential breakage if a customer is using a full profile as a secret but it sounds like we can accept that?

JordanMontgomery avatar Oct 22 '25 15:10 JordanMontgomery

There is still the potential breakage if a customer is using a full profile as a secret but it sounds like we can accept that?

That's correct.

marko-lisica avatar Oct 23 '25 10:10 marko-lisica