amperity / vault-clj

Clojure client for Hashicorp's Vault secret management system.
Other
70 stars 17 forks source link

Switch :form-params to :body for http-kit conversion #62

Closed greglook closed 2 years ago

greglook commented 2 years ago

After upgrading to 1.1.0 to add support for the orphan token endpoint, we discovered that services were failing to authenticate with app-role with the new version of the library. This upgrade crossed the http-kit switchover, so I suspected that to be the cause. We were seeing errors like the following:

Vault API server errors: failed to parse JSON input: invalid character 'r' looking for beginning of value
{:type :vault.client.api-util/api-error,
 :status 400,
 :errors "failed to parse JSON input: invalid character 'r' looking for beginning of value"}

This is suspicious because one of the posted fields is role_id, which also begins with the letter "r". After a little investigation, I found that http-kit always turns :form-params into a query-string style body whereas clj-http would up-convert them into JSON when the content type was specified. This means that the vault server was trying to parse role_id=123...&secret_id=456... as JSON, which obviously fails with the observed error.

To fix this, switch all references to :form-params to straight :body data, which there is already logic to serialize as JSON in the API helper code. I tested this against our actual Vault server and was able to authenticate with app-role:

=> (vault/authenticate! client :app-role {:role-id "REDACTED", :secret-id "REDACTED"})
#vault.client.http.HTTPClient
{:api-url "REDACTED",
 :auth #<Atom@992adc5 {:accessor "REDACTED",
                       :client-token "REDACTED",
                       :entity-id "981472a8-a326-1dd2-7a7a-0db0b821b7b0",
                       :lease-duration 14400,
                       :metadata {:role-name "tenant-service-aws-dev"},
                       :orphan true,
                       :policies ["default" "tenant-service.aws-dev"],
                       :renewable true,
                       :token-policies ["default" "tenant-service.aws-dev"],
                       :token-type "service",
                       :vault.lease/expiry #inst "2022-01-31T22:52:34.852Z"}>,
 :http-opts nil,
 :lease-check-jitter 20,
 :lease-check-period 60,
 :lease-renewal-window 600,
 :lease-timer #<java.lang.Thread@41f2ed8a Thread[vault-lease-timer,5,main]>,
 :leases #<Atom@5a21200f {}>}
codecov-commenter commented 2 years ago

Codecov Report

Merging #62 (4bd7049) into master (a8f5647) will not change coverage. The diff coverage is 36.84%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   55.85%   55.85%           
=======================================
  Files          10       10           
  Lines         734      734           
  Branches       27       27           
=======================================
  Hits          410      410           
  Misses        297      297           
  Partials       27       27           
Impacted Files Coverage Δ
src/vault/client/http.clj 25.74% <0.00%> (ø)
src/vault/authenticate.clj 51.69% <63.63%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a8f5647...4bd7049. Read the comment docs.