Fix the unknown secret and credential escaping in gitlab_group_dependency_proxy

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/terraform-provider-gitlab:jmrp-tf-dependency-proxy into main

What does this MR do and why?

This MR fixes three bugs in gitlab_group_dependency_proxy, and changes only resource_gitlab_group_dependency_proxy.go and its test file. The first two commits fix the first two bugs, and the third runs their new tests against a GitLab instance instead of the stand-in server the first two added. The last four come from review: the fourth changes how the second stores an unconfigured secret, the fifth fixes the third bug, in Read, the sixth makes Read store the secret as the fourth does, and the seventh tests an update that keeps the proxy disabled.

A disabled proxy can't be created. secret is optional and computed, and ValidateConfig rejects it when enabled is false, so the only valid configuration for a disabled proxy leaves it out. On create its planned value is then unknown, and modelToStateModel keeps it because GitLab never returns the secret (resource_gitlab_group_dependency_proxy.go#L294-299). Terraform fails the apply with Provider returned invalid result object after apply, stores the resource as tainted, and every later apply replaces it and fails again. After the mutation the secret is now set with types.StringValue(data.Secret.ValueString()), so an unknown secret is stored as an empty string, and a configured secret is kept as before. The second commit stored it as null, and the fourth stores it as the review suggested. identity needs no change, because it is always set from the mutation's response. This is #6906 (closed), which prunid reported with an analysis that points at the same function, and offered to fix.

The credentials are written into the GraphQL query without escaping. updateDependencyProxySettings builds the mutation with fmt.Sprintf and puts identity and secret between quotes as they are (resource_gitlab_group_dependency_proxy.go#L304-318), so the GraphQL parser decodes them:

  • A secret with a backslash escape that is valid in a GraphQL string reaches GitLab as a different value and the apply succeeds: pa\"ss\\word arrives as pa"ss\word. The secret is never read back, so no plan shows the difference.
  • The same escape in identity makes the apply fail with Provider produced inconsistent result after apply once the decoded identity has been stored, and on create leaves the resource tainted.
  • A lone ", or a \ that doesn't start a GraphQL escape, makes the query invalid. The apply then fails with the same inconsistent result error rather than the parse error, which comes back in the errors of an HTTP 200 answer that the resource doesn't read.

The first commit sends the group path, enabled, identity and secret as GraphQL variables, typed as the mutation's input declares them (ID!, Boolean, String, String), the way gitlab_group_dependency_proxy_image_ttl_policy already sends its own.

A disabled proxy isn't read. Read set enabled and identity from GitLab only when the proxy was enabled, and otherwise kept what the state held (resource_gitlab_group_dependency_proxy.go#L218-223). A proxy disabled outside Terraform therefore stayed enabled in the state, the plan showed no changes, and no apply enabled it again. Importing a disabled proxy left enabled and identity unset. The fifth commit sets both from GitLab's answer whatever enabled reads. The secret is still kept from the state, because GitLab never returns it, and the sixth commit sets it there with types.StringValue(data.Secret.ValueString()) as well: an import left it null where Create stores an empty string, the plan kept the null, and an update that kept an imported proxy disabled failed with Provider produced inconsistent result after apply once Update stored an empty string.

Behaviour change: an identity or secret that holds \", \\, \n or another GraphQL escape now reaches GitLab exactly as configured, instead of decoded by the GraphQL parser. A secret stored decoded by an earlier version stays so in GitLab until the resource next sends it, because the provider can't read the secret back to see the difference. A value that used to break the query now works. A proxy disabled outside Terraform now shows in the next plan as an update, which enables it again, and an imported disabled proxy reads enabled = false and the identity GitLab holds. An imported proxy holds an empty secret rather than null, as one created without a secret does.

The three new acceptance tests run against a group of the GitLab instance, like the other tests in the file:

  • TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences applies an identity and a secret that hold backslash escapes. GitLab returns the identity it stored, so the test checks that it comes back unchanged. It never returns the secret, so the secret also holds a quote, which the query on main can't parse.
  • TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials creates the resource with enabled = false, checks that the secret is stored as an empty string, imports the resource and checks that the import reads the same secret, enables the proxy through the GraphQL API and checks that applying the same configuration, which keeps enabled = false, plans an update that disables it again, and then enables the proxy with credentials.
  • TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform creates an enabled proxy, disables it through the GraphQL API, and checks that applying the same configuration again plans an update.

I found the first two bugs while building an MCP server for GitLab.

Closes #6906 (closed)

Closes #6909 (closed)

How to verify

make testacc-up
make testacc RUN=TestAccGitlabGroupDependencyProxy_

With the resource as it is on main, the three new tests fail: the escape test with Provider produced inconsistent result after apply, because GitLab refuses the query as malformed in an HTTP 200 answer the resource doesn't read, the disabled test with Provider returned invalid result object after apply, and the test that disables the proxy outside Terraform with expected Update, got action(s): [no-op]. With this MR the six tests in the file pass, _basic and _import included, which send the new mutation to GitLab.

What I ran after the second review (Go 1.26.5, Terraform 1.4.0, GitLab CE 19.5.0)

The disabled test as it is now, with the resource as it is at the fifth commit (fe638ae7):

    resource_gitlab_group_dependency_proxy_test.go:244: Step 2/4 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.

          map[string]string{
        -     "secret": "",
          }
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (2.76s)

A local test that imports a disabled proxy, enables it through the GraphQL API and applies the same configuration, which keeps it disabled, fails at fe638ae7 and passes with this branch:

Step 2/2 error: Error running apply: exit status 1

Error: Provider produced inconsistent result after apply

When applying changes to gitlab_group_dependency_proxy.foo, provider
"provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
new value: .secret: inconsistent values for sensitive attribute.

With Update changed to always send enabled: true, the new step of the disabled test fails, and the other five tests pass:

Step 3/4 error: Error running apply: exit status 1

Error: Provider produced inconsistent result after apply

When applying changes to gitlab_group_dependency_proxy.foo, provider
"provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
new value: .enabled: was cty.False, but now cty.True.

This branch:

--- PASS: TestAccGitlabGroupDependencyProxy_import (3.07s)
--- PASS: TestAccGitlabGroupDependencyProxy_validation (2.27s)
--- PASS: TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences (3.31s)
--- PASS: TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform (4.33s)
--- PASS: TestAccGitlabGroupDependencyProxy_basic (4.80s)
--- PASS: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (5.01s)
PASS

make test, make lint-golangci (0 issues), lint-tfprovider, lint-custom, make generate (no changes), go vet -tags acceptance and gofmt -s pass, at the sixth commit and at the seventh.

What I ran after the first review (Go 1.26.5, Terraform 1.4.0, GitLab CE 19.5.0)

The three new tests with the resource as it is on main (3204e99a):

=== NAME  TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences
    resource_gitlab_group_dependency_proxy_test.go:218: Step 1/1 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to gitlab_group_dependency_proxy.foo, provider
        "provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
        new value: .enabled: was cty.True, but now cty.False.

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

        Error: Provider produced inconsistent result after apply

        When applying changes to gitlab_group_dependency_proxy.foo, provider
        "provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
        new value: .identity: was cty.StringVal("some\\\\identity"), but now
        cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
=== NAME  TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials
    resource_gitlab_group_dependency_proxy_test.go:244: Step 1/3 error: Error running apply: exit status 1

        Error: Provider returned invalid result object after apply

        After the apply operation, the provider still indicated an unknown value for
        gitlab_group_dependency_proxy.foo.secret. All values must be known after
        apply, so this is always a bug in the provider and should be reported in the
        provider's own repository. Terraform will still save the other known object
        values in the state.
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (14.85s)
=== NAME  TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform
    resource_gitlab_group_dependency_proxy_test.go:292: Step 2/2 error: Pre-apply plan check(s) failed:
        'gitlab_group_dependency_proxy.foo' - expected Update, got action(s): [no-op]
--- FAIL: TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences (20.26s)
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform (30.89s)
FAIL

Each review change also fails its test without it. With the resource as it is at aefa0c1c, the disabled test fails its first step with Attribute 'secret' not found, because the secret was stored as null. With the resource as it is at the fourth commit (24b3f59b), the two tests that read a disabled proxy fail:

=== NAME  TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials
    resource_gitlab_group_dependency_proxy_test.go:244: Step 2/3 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.

          map[string]string{
        -     "enabled":  "false",
        -     "identity": "",
          }
=== NAME  TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform
    resource_gitlab_group_dependency_proxy_test.go:292: Step 2/2 error: Pre-apply plan check(s) failed:
        'gitlab_group_dependency_proxy.foo' - expected Update, got action(s): [no-op]
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (32.40s)
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform (34.13s)

This branch:

--- PASS: TestAccGitlabGroupDependencyProxy_import (43.00s)
--- PASS: TestAccGitlabGroupDependencyProxy_validation (3.69s)
--- PASS: TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences (58.96s)
--- PASS: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (63.10s)
--- PASS: TestAccGitlabGroupDependencyProxy_disabledOutsideTerraform (69.14s)
--- PASS: TestAccGitlabGroupDependencyProxy_basic (82.19s)
PASS

I also took out each review change in turn: without the types.StringValue line the disabled test fails with Provider returned invalid result object after apply, and with identity read only while the proxy is enabled its import step fails on identity. make test, make lint-golangci (0 issues), lint-tfprovider, lint-custom, make generate (no changes), go vet -tags acceptance and gofmt -s pass.

What I ran before the review, on aefa0c1c (Go 1.26.5, Terraform 1.4.0 for the tests and 1.13.4 for the CLI run, GitLab EE 19.5.0 without a license)

The two tests the first commits added, with the resource as it is on main (3204e99a):

=== NAME  TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences
    resource_gitlab_group_dependency_proxy_test.go:217: Step 1/1 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to gitlab_group_dependency_proxy.foo, provider
        "provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
        new value: .identity: was cty.StringVal("some\\\\identity"), but now
        cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

        Error: Provider produced inconsistent result after apply

        When applying changes to gitlab_group_dependency_proxy.foo, provider
        "provider[\"registry.terraform.io/hashicorp/gitlab\"]" produced an unexpected
        new value: .enabled: was cty.True, but now cty.False.

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
=== NAME  TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials
    resource_gitlab_group_dependency_proxy_test.go:243: Step 1/2 error: Error running apply: exit status 1

        Error: Provider returned invalid result object after apply

        After the apply operation, the provider still indicated an unknown value for
        gitlab_group_dependency_proxy.foo.secret. All values must be known after
        apply, so this is always a bug in the provider and should be reported in the
        provider's own repository. Terraform will still save the other known object
        values in the state.
--- FAIL: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (3.13s)
--- FAIL: TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences (3.16s)
FAIL

The mutation main builds for that configuration, sent to the same instance, is answered with HTTP 200 and {"errors":[{"message":"Expected string or block string, but it was malformed","locations":[{"line":6,"column":27}]}]}. Each test also fails on its own at the commit before its fix: the escape test on main, and the disabled test on top of the first commit, with the same unknown value error.

The branch at aefa0c1c:

--- PASS: TestAccGitlabGroupDependencyProxy_import (5.17s)
--- PASS: TestAccGitlabGroupDependencyProxy_validation (1.93s)
--- PASS: TestAccGitlabGroupDependencyProxy_credentialsWithEscapeSequences (3.96s)
--- PASS: TestAccGitlabGroupDependencyProxy_disabledWithoutCredentials (4.74s)
--- PASS: TestAccGitlabGroupDependencyProxy_basic (5.79s)
PASS

I also took out each part of the change in turn and ran the two tests against the same instance: three changes to how the secret was stored as null (since replaced by the fourth commit), embedding identity or secret in the query again, and dropping the enabled variable. Each of the six made at least one of the tests fail.

End to end with the Terraform CLI, against a stand-in for the GitLab API that parses, validates and executes every GraphQL request with graphql-core against GitLab's GraphQL schema as introspected from GitLab.com (19.5.0-pre), so the variables and their types are checked against the real input type and a value written into the query is decoded by a GraphQL parser:

  • enabled = false and nothing else: provider 19.5.0 and a build of main fail the apply with Provider returned invalid result object after apply and leave the resource tainted. A build of the branch creates it with a null secret, the next plan reports no changes, and enabling the proxy with credentials afterwards works.
  • secret = "pa\\\"ss\\\\word" (the value pa\"ss\\word): 19.5.0 applies, the stand-in receives pa"ss\word, and the next plan reports no changes. The branch sends pa\"ss\\word.
  • identity = "some\\\\identity": 19.5.0 sends some\identity, and the apply fails with .identity: was cty.StringVal("some\\\\identity"), but now cty.StringVal("some\\identity") and leaves the resource tainted. The branch sends some\\identity.
  • secret = "pa\"ss": the stand-in refuses the query 19.5.0 sends with Syntax Error: Unterminated string., and the apply fails with Provider produced inconsistent result after apply on enabled and identity. The branch sends pa"ss.

make test, make lint-golangci (0 issues), lint-tfprovider, lint-custom, go vet -tags acceptance and gofmt -s pass.

Related merge requests

!3201 (merged) moved gitlab_compliance_framework from values written into the query to GraphQL variables, the change the first commit makes here.

Edited by José M. Requena Plens

Merge request reports