* add hashfunc field to EntryFormatter struct and adjust NewEntryFormatter function and tests
* add HeaderAdjuster interface and require it in EntryFormatter
dquote> adjust all references to NewEntryFormatter to include a HeaderAdjuster parameter
* replace use of hash function in AuditedHeadersConfig's ApplyConfig method with Salter interface instance
* fixup! replace use of hash function in AuditedHeadersConfig's ApplyConfig method with Salter interface instance
* review feedback
* Go doc typo
* add another test function
---------
Co-authored-by: Peter Wilson <peter.wilson@hashicorp.com>
* OpenAPI: Define default response structure for ListOperations
Almost all Vault ListOperation responses have an identical response
schema. Update the OpenAPI generator to know this, and remove a few
instances where that standard response schema had been manually
copy/pasted into place in individual endpoints.
* changelog
* Only render StandardListResponse schema, if an operation uses it
* Teach the response schema validation test helper about the default list schema too
---------
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
* begin refactoring of event package into audit package
* audit options additions
* rename option structs
* Trying to remove 'audit' from the start of names.
* typo
* typo
* typo
* newEvent required params
* typo
* comments on noop sink
* more refactoring - merge json/jsonx formatters
* fix file backend and tests
* Moved unexported funcs to formatter, fixed file tests
* typos, comments, moved func
* fix corehelpers
* fix backends (syslog, socket)
* Moved some sinks back to generic event package.
* return of the file sink
* remove unneeded sink params/return vars
* Implement Register and Deregister Audit Devices for EventLogger Framework (#21940)
* add function to create StdoutSinkNode
* add boolean argument to audit Factory function
* create eventlogger nodes in backend factory functions
* simplify NewNoopSink function and remove DiscardSinkNode
* make the sanity test in the file backend mutually exclusive based on useEventLogger value
* remove test cases that no longer made sense and were failing
* NewFileSink attempts to open file for sanity check
* fix FileSink tests and update FileSink to remove discard, stdout but add /dev/null
* Moved WithPrefix from FileSink to EventFormatter
* move prefix in backend
* NewFormatterConfig and Options (tests fixed)
* Little tidy up
* add test where audit file is created with useEventLogger set to true
* only create eventlogger.Node instances when useEventLogger is true
fix failing test due to invalid string conversion of FileMode value
* moved variable definition to more appropriate scope
---------
Co-authored-by: Marc Boudreau <marc.boudreau@hashicorp.com>
* Add missing `Query: true` metadata to API definitions
Also improve the documentation comment for `Query` to guide people better how they should be setting `Query` in the future.
Endpoints affected:
- auth/approle/role/{role_name}/secret-id/destroy
- auth/approle/role/{role_name}/secret-id-accessor/destroy
- auth/token/lookup
- auth/token/lookup-self
- sys/internal/specs/openapi
- sys/wrapping/lookup
- identity/oidc/provider/{name}/authorize
There are also endpoints in the `aws` and `gcp` secrets engines which need the same treatment in their own PRs.
When working on the `auth/token/lookup-self` path, I discovered that it
had a parameter which was completely pointless - it was even documented
as unused. It only existed because the `auth/token/lookup-self` code
path was implemented by bodging the current token into the request data
and passing control to the `auth/token/lookup` handler directly -
instead of just factoring out the common code to a reusable function -
so I fixed that whilst I was there.
Note that two of the affected endpoints currently have one form of their
OpenAPI operation ID set to something mentioning "with-parameters":
- identity/oidc/provider/{name}/authorize
- sys/internal/specs/openapi
These operation IDs should be changed, as they perpetuate
a misunderstanding - both read (GET) and update (POST/PUT) forms of
these APIs are **equally** capable of being used with parameters.
* I failed to spot that the aws plugin is in-repo! Update that too.
* Remove code cleanup changes from this PR
* Wording and wrapping adjustment as requested.
* add useEventLogger argument to audit Factory functions
* adjusting Factory functions defined in tests
* fixup! adjusting Factory functions defined in tests
* CreateOperation should only be implemented alongside ExistenceCheck
Closes#12329
Vault treats all POST or PUT HTTP requests equally - they default to
being treated as UpdateOperations, but, if a backend implements an
ExistenceCheck function, CreateOperations can be separated out when the
existence check returns false.
It follows, then, that if a CreateOperation handler is implemented
without an ExistenceCheck function, this is unreachable code - a coding
error. It's a fairly minor error in the grand scheme of things, but it
causes the generated OpenAPI spec to include x-vault-createSupported for
operations on which create can never actually be invoked - and promotes
muddled understanding of the create/update feature.
In this PR:
1) Implement a new test, which checks all builtin auth methods and
secrets engines can be successfully initialized. (This is important
to validate the next part.)
2) Expand upon the existing coding error checks built in to
framework.Backend, adding a check for this misuse of CreateOperation.
3) Fix up instances of improper CreateOperation within the Vault
repository - just two, transit and mock.
Note: At this point, the newly added test will **fail**.
There are improper uses of CreateOperation in all of the following:
vault-plugin-auth-cf
vault-plugin-auth-kerberos
vault-plugin-auth-kubernetes
vault-plugin-secrets-ad
vault-plugin-secrets-gcpkms
vault-plugin-secrets-kubernetes
vault-plugin-secrets-kv
vault-plugin-secrets-openldap
vault-plugin-secrets-terraform
each of which needs to be fixed and updated in go.mod here, before this
new check can be added.
* Add subtests
* Add in testing of KV v2, which otherwise doesn't get tested
This is a surprisingly complicated special case
* The database plugin needs special handling as well, and add in help invocations of the builtin backends too
* Fix extra package prefix
* Add changelog
* Update 6 out of 9 plugins to needed new versions
Note, this IS an upgrade despite the apparent version numbers going
down. (That's a consequence of slightly odd release management occurring
in the plugin repositories.)
* Update to deal with code changes since branch originally created
* Perform necessary update of vault-plugin-secrets-kubernetes so that CI checks on PR can run
* Fix another instance of incorrect CreateOperation, for a test-only endpoint
By being hidden behind a Go build constraint, it had evaded notice until
now.
* Add an opportunistic test of sys/internal/specs/openapi too
* Fix ACME tidy to not reference acmeCtx
acmeContext is useful for when we need to reference things with a ACME
base URL, but everything used in tidy doesn't need this URL as it is not
coming from an ACME request.
Refactor tidy to remove references to acmeContext, including dependent
functions in acme_state.go.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Remove spurious log message
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Draft Tidy Acme Test with Backdate Storage + Backdate Sysxsx
* Fixes to ACME tidy testing
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Correctly set account kid to update account status
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add TestTidyAcmeWithSafetyBuffer
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add test for disabling tidy operation
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add acme_account_safety_buffer to auto-tidy config
Resolve: #21872
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add tests verifying tidy safety buffers
Co-authored-by: kitography <khaines@mit.edu>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add changelog entry
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add account status validations and order cleanup tests
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: kitography <khaines@mit.edu>
Co-authored-by: Steve Clark <steven.clark@hashicorp.com>
* imprv: Add a parameter to allow ExtKeyUsage field usage from a role
* chore: Add the changelog entry
* imprv: Reword UI and changelog
* doc: Add allow_role_extkeyusage in parameter list
* imprv: Align variable names with existing fields/codebase
* Add unit test and tweak some labels
---------
Co-authored-by: Steve Clark <steven.clark@hashicorp.com>
* Refactor CRL writing config to passthrough cache
When reading the CRL config via API endpoint, always read through to the
disk, updating the cache in the process. Similarly, when writing to the
CRL config, read first from disk (updating the cache), and on write,
also write back through the cache, providing consistency without the
need to invalidate through markConfigDirty(...).
This will form the basis of the new pattern for config caching.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Refactor ACME writing config to passthrough cache
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
- Updating existing entries in OSS so that this can be backported.
- The proper fix on main going forward it to remove these ENT only
entries and move them into the enterprise only file
* OpenAPI: Separate ListOperation from ReadOperation
Historically, since Vault's ReadOperation and ListOperation both map to
the HTTP GET method, their representation in the generated OpenAPI has
been a bit confusing.
This was partially mitigated some time ago, by making the `list` query
parameter express whether it was required or optional - but only in
a way useful to human readers - the human had to know, for example, that
the schema of the response body would change depending on whether `list`
was selected.
Now that there is an effort underway to automatically generate API
clients from the OpenAPI spec, we have a need to fix this more
comprehensively. Fortunately, we do have a means to do so - since Vault
has opinionated treatment of trailing slashes, linked to operations
being list or not, we can use an added trailing slash on the URL path to
separate list operations in the OpenAPI spec.
This PR implements that, and then fixes an operation ID which becomes
duplicated, with this change applied.
See also hashicorp/vault-client-go#174, a bug which will be fixed by
this work.
* Set further DisplayAttrs in auth/github plugin
To mask out more duplicate read/list functionality, now being separately
generated to OpenAPI client libraries as a result of this change.
* Apply requested changes to operation IDs
I'm not totally convinced its worth the extra lines of code, but
equally, I don't have strong feelings about it, so I'll just make the
change.
* Adjust logic to prevent any possibility of generating OpenAPI paths with doubled final slashes
Even in the edge case of improper use of regex patterns and operations.
* changelog
* Fix TestSudoPaths to pass again... which snowballed a bit...
Once I looked hard at it, I found it was missing several sudo paths,
which led to additional bug fixing elsewhere.
I might need to pull some parts of this change out into a separate PR
for ease of review...
* Fix other tests
* More test fixing
* Undo scope creep - back away from fixing sudo paths not shown as such in OpenAPI, at least within this PR
Just add TODO comments for now.
- Add a little more debug to try and understand how we could get the
following error I saw within GHA
```
path_tidy_test.go:769: Created root and leaf certificate, deleted leaf, but a got a certificate count of 2
```
- Fix some other issues discovered while running the test through
stress locally
To allow us to support CIEPS backend state, allow the backend to
contain enterprise only state variables. Also allow us to implement
enterprise only hooks into the various backend functions to initialize,
periodicFunc, cleanup and invalidate.
* Add backend format linting to pre-commit hook
By taking a slight penalty with each commit, we can ensure that
contributors follow the format behavior by default (if they run hooks),
making accidental PRs without proper formatting less likely.
Additionally, fix gofmtcheck to align with the Makefile, fixing the
corresponding fmtcheck target for use with the hook.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Fix formatting errors
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
- There's a race within the Plugin reloading mechanism that isn't
trivial to address. To silence some of the failures, switch this
test to use sealing of the cores instead of the plugin reload
mechanism
- We've seen a few issues with bind's auto-loading of configuration
too quickly at bad times leading to it having partial configurations
or not all files/permissions being restored properly during it's read
attempt.
- See if the freeze/thaw rndc commands will help out with these timing
issues
* Fix OpenAPI spec definitions for PKI EAB APIs
- Do not generate duplicate operation ids for the various new-eab apis
- Fill out proper operation verb for eab delete call
- Pluralize operation verb for list-eab-keys api
- Fill out proper response data for new-eab and list-eab-keys
* Add cl
* openapi: Fix response schema for PKI Issue requests
* tests
* changelog
* another expiration for generate/rotate root
* more type fixes from @stevendpclark
The upcoming event main plugin will use a very similar pattern
as the database plugin map, so it makes sense to refactor this and move
this map out. It also cleans up the database plugin backend so that
it does not have to keep track of the lock.
Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
* Replace all time.ParseDurations with testutil.ParseDurationSeconds
* Changelog
* Import formatting
* Import formatting
* Import formatting
* Import formatting
* Semgrep rule that runs as part of CI
* Supporting PR for Enterprise ACME PR cluster tests
- Some changes within the OSS test helpers to help in the ACME Enterprise test cases.
* Don't rename existing helper method to make oss/ent merge easier
* Adds automated ACME tests using Caddy.
* Do not use CheckSignatureFrom method to validate TLS-ALPN-01 challenges
* Uncomment TLS-ALPN test.
* Fix validation of tls-alpn-01 keyAuthz
Surprisingly, this failure was not caught by our earlier, but unmerged
acme.sh tests:
> 2023-06-07T19:35:27.6963070Z [32mPASS[0m builtin/logical/pkiext/pkiext_binary.Test_ACME/group/acme.sh_tls-alpn (33.06s)
from https://github.com/hashicorp/vault/pull/20987.
Notably, we had two failures:
1. The extension's raw value is not used, but is instead an OCTET
STRING encoded version:
> The extension has the following ASN.1 [X.680] format :
>
> Authorization ::= OCTET STRING (SIZE (32))
>
> The extnValue of the id-pe-acmeIdentifier extension is the ASN.1
> DER encoding [X.690] of the Authorization structure, which
> contains the SHA-256 digest of the key authorization for the
> challenge.
2. Unlike DNS, the SHA-256 is directly embedded in the authorization,
as evidenced by the `SIZE (32)` annotation in the quote above: we
were instead expecting this to be url base-64 encoded, which would
have a different size.
This failure was caught by Matt, testing with Caddy. :-)
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Quick gofmt run.
* Fix challenge encoding in TLS-ALPN-01 challenge tests
* Rename a PKI test helper that retrieves the Vault cluster listener's cert to distinguish it from the method that retrieves the PKI mount's CA cert. Combine a couple of Docker file copy commands into one.
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: Steve Clark <steven.clark@hashicorp.com>
Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Correctly validate ACME PoP against public key
ACME's proof of possession based revocation uses a signature from the
private key, but only sends the public copy along with the request.
Ensure the public copy matches the certificate, instead of failing to
cast it to a private key.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add ACME revocation tests
* Clarify commentary in acmeRevocationByPoP
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: Steve Clark <steven.clark@hashicorp.com>
* Allow CSRs with basic constraint extension with IsCA=false
- We previously forbid any CSR with a basic constraint extension within the CSR.
- It was discovered that some ACME clients (Proxmox ACME client) do send us this extension with a value of IsCA to false.
- So allow the extension to be set within the ACME CSR with
a value of IsCA set to false
- Add a test for both the IsCA=true and IsCA=false use-cases and make sure we don't actually set the extension back within the generated certificate.
* PR feedback
- Move basic constraint function to sdk, increase test coverage
- Error out on extra characters being returned from the asn1 unmarshalling.
* make fmt
- When running the SubtestACMEStepDownNode by itself we would be sealing the active node within the cluster too quickly and would end up with the other nodes failing to become an active node with the message: not part of stable configuration, aborting election
- Add an extra check that the raft autopilot state is healthy and that FailureToTolerance has a value of 1 or higher before letting the test continue.
* Signal ACME challenge engine if existing challenges were loaded
- Addresses an issue of existing challenges on disk not being processed until a new challenge is accepted when Vault restarts
- Move loading of existing challenges from the plugin's initialize method into the challenge engine's thread
- Add docker test that validates we addressed the issue and ACME works across standby nodes.
* Add cl
* Adds an ACME validation failure test for certbot that doesn't run in CI unless a particular regression test env var is provided. Also includes a helper function to determine whether or not CI is running and if the regression test env var is provided.
* Rename and move the local or regression test env check. Sinkhole our invalid domain for ACME certbot test to avoid spamming someone's domain if it's registered in the future.
* Add ACME TLS-ALPN-01 Challenge validator to PKI
This adds support for verifying the last missing challenge type,
TLS-ALPN-01 challenges, using Go's TLS library. We wish to add this as
many servers (such as Caddy) support transparently renewing certificates
via this protocol, without influencing the contents of sites served.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Enable suggesting, validating tls-alpn-01 in PKI
Notably, while RFC 8737 is somewhat vague about what identifier types
can be validated with this protocol, it does restrict SANs to be only
DNSSans; from this, we can infer that it is not applicable for IP
typed identifiers. Additionally, since this must resolve to a specific
domain name, we cannot provision it for wildcard identifiers either.
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Fix test expectations to allow ALPN challenges
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add tls-alpn-01 as a supported challenge to docs
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add test for tls-alpn-01 challenge verifier
This hacks the challenge engine to allow non-standard (non-443) ports,
letting us use a local server listener with custom implementation.
In addition to the standard test cases, we run:
- A test with a longer chain (bad),
- A test without a DNSSan (bad),
- A test with a bad DNSSan (bad),
- A test with some other SANs (bad),
- A test without a CN (good),
- A test without any leaf (bad), and
- A test without the extension (bad).
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Add changelog entry
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
* Update builtin/logical/pki/acme_challenges.go
Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>
---------
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Co-authored-by: Kit Haines <khaines@mit.edu>
When writing DNS configs, make sure to push the zone file prior to
writing the reference to the zone in the named.conf.options file.
Otherwise, when adding the initial domain (or any subsequent domains,
which isn't really used by these tests), a race occurs between Docker,
writing the updated config files, and the bind daemon, detecting that
mtime has changed on the named.conf.options file and reloading it
and any referenced zone files.
This should fix the error seen in some tests:
> /etc/bind/named.conf:9: parsing failed: file not found
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
