Skip to content

Conversation

aahel
Copy link

@aahel aahel commented Oct 8, 2025

Description

Currently if the user does not set deny_null_bind in tf config we send false to the vault api. This creates an insecure config.
In this pr we are setting deny_null_bind to true if its not provided in tf config
This prevents users from bypassing authentication when providing an empty password. It also makes it compliant with vault's default behaviour.

jira:- https://hashicorp.atlassian.net/browse/SECVULN-29861

Manual testing

Testing that deny_null_bind gets set to true when not provided in tf config

terraform {
  required_providers {
    vault = {
      source = "hashicorp/vault"
      version = "~> 4.0"
    }
  }
}

provider "vault" {
  address = var.address
  token   = var.token
}

# Minimal LDAP auth backend to test deny_null_bind default behavior
resource "vault_ldap_auth_backend" "test" {
  path = "ldap-test"
  url  = "ldap://test.example.com:389"
  
  # Minimal required config
  binddn   = "cn=admin,dc=test,dc=com"
  bindpass = "test-password"
  
  # deny_null_bind is NOT specified - should default to true
}

# Output to verify the default behavior
output "deny_null_bind_value" {
  value = vault_ldap_auth_backend.test.deny_null_bind
  description = "Should show 'true' due to secure default"
}

output "ldap_backend_info" {
  value = {
    path           = vault_ldap_auth_backend.test.path
    accessor       = vault_ldap_auth_backend.test.accessor
    deny_null_bind = vault_ldap_auth_backend.test.deny_null_bind
  }
}

Before the change

Screenshot 2025-10-08 at 4 22 52 PM

After the Change

Screenshot 2025-10-08 at 4 16 25 PM Screenshot 2025-10-08 at 5 20 03 PM

Checklist

  • Added CHANGELOG entry (only for user-facing changes)
  • Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

Screenshot 2025-10-08 at 5 07 10 PM

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

@aahel aahel requested review from a team as code owners October 8, 2025 05:08
@aahel aahel requested a review from raeka-attari October 8, 2025 05:08
@aahel aahel marked this pull request as draft October 8, 2025 05:08
@aahel aahel marked this pull request as ready for review October 8, 2025 12:23
@aahel aahel self-assigned this Oct 8, 2025
@fairclothjm
Copy link
Contributor

@aahel Hello, thanks for the PR! I am wondering why this doesn't work without your change. Everything looks correct:

  1. Vault API returns deny_null_bind in the response
  2. TFVP sets the field to Computed, so an unset config entry should use the Value returned from Vault
  3. TFVP sets the state based on the READ

Do you know why this isn't working?

@fairclothjm
Copy link
Contributor

Oh, I think this is happening because we are ignoring the error here

That said, I think setting the Default in TFVP is ok in this case.

@aahel
Copy link
Author

aahel commented Oct 16, 2025

@aahel Hello, thanks for the PR! I am wondering why this doesn't work without your change. Everything looks correct:

  1. Vault API returns deny_null_bind in the response
  2. TFVP sets the field to Computed, so an unset config entry should use the Value returned from Vault
  3. TFVP sets the state based on the READ

Do you know why this isn't working?

this was making it send false (default value of bool field) to the api.

Now using CustomizeDiff i am forcing the deny_null_bind field in config to be true if field was missing in config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants