hashicorp / vault-plugin-secrets-azure

Vault Azure Secrets plugin

Mozilla Public License 2.0

26 stars 20 forks source link

Correct nil check when unassigning roles #210

Closed jmarion-arista closed 2 months ago

jmarion-arista commented 2 months ago

This is a continued fix from hashicorp/vault-plugin-secrets-azure#191 (reported in hashicorp/vault-plugin-secrets-azure#190)

The issue with the original fix is that Go will still try to evaluate the right side of the || operation and try to dereference .StatusCode of a nil rawResponse.

Here is a Go playground PoC whipped up by my colleague: https://go.dev/play/p/tu6hOZgv7ZT

Fixes

hashicorp/vault-plugin-secrets-azure#190

hashicorp-cla-app[bot] commented 2 months ago

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

hashicorp-cla-app[bot] commented 2 months ago

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

Foxboron commented 2 months ago

Cool, thanks for the fix :)

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Signing CLAs for contributions that can't be considered "original work" under copyright is a bit weird.

Foxboron commented 2 months ago

Any updates on this @vinay-gopalan and @hsimon-hashicorp?

Andrei-Predoiu commented 2 months ago

We're also waiting on this one :)

jmarion-arista commented 2 months ago

We have a corporate CLA in place for this but I can't get the GH check to recognize this. Our Vault Enterprise instance has been crashing pretty consistently for a while now so we really need this small fix merged.

vinay-gopalan commented 2 months ago

Hi, sorry for the delay in responding, and thanks for opening this PR!

We have a separate PR open that now correctly parses the error from Azure based on their documentation, and appropriately handles the error. A validation test for resolving the panic was also included. Going to close this PR for now, but will also update this thread with updates on the status of the bug fix. Apologies once again for the inconvenience this caused, and thanks for your patience 🙏🏼