Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make reattest to renew default behaviour #4791

Merged
merged 64 commits into from
Feb 7, 2024

Conversation

faisal-memon
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality

How agent handles certificate expiration

Description of change

Make reattest default behavior for renewing svid for environments that support reattesting.

Which issue this PR fixes

fixes #4658

@faisal-memon
Copy link
Contributor Author

Looks like the x509 pop tests are failing because they need to reattest now.

[2024-01-11T21:14:05Z] running x509pop test...
2024/01/11 21:14:06 Watching X.509 contexts
2024/01/11 21:14:06 Attesting agent...
2024/01/11 21:14:06 Renewing agent...
2024/01/11 21:14:06 Node attestation client failed: failed to renew agent: failed to renew agent: rpc error: code = PermissionDenied desc = agent can't renew SVID, must reattest
[2024-01-11T21:14:06Z] failed to check x509pop attestion
[2024-01-11T21:14:06Z] step 04-test-x509pop-attestation failed

@faisal-memon
Copy link
Contributor Author

I think its this section here:

// Renew agent
if err := x509popRenew(ctx, svidResp); err != nil {
return fmt.Errorf("failed to renew agent: %w", err)
}

We should either remove or invert the logic so that we expect to get an error back.

@azdagron azdagron added this to the 1.9.0 milestone Jan 16, 2024
@faisal-memon
Copy link
Contributor Author

Seems like a transient failure:

tar xvf images.tar.gz
  make load-images
  shell: /usr/bin/bash -e {0}
  env:
    GO_VERSION: 1.21.[5](https://github.com/spiffe/spire/actions/runs/7546181593/job/20543643786?pr=4791#step:8:5)
oidc-discovery-provider-image.tar
spire-agent-image.tar
spire-server-image.tar

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now

@MarcosDY
Copy link
Collaborator

we are making reattest default, but we are not allowing a way to disable this (in case someone get into an error and they must disable this feature),
what do you think about adding a deprecated configuration to disable reattestation? and we can remove that configuration in 1.10?

if attestedNode.CanReattest && fflag.IsSet(fflag.FlagReattestToRenew) {
return nil, errorutil.PermissionDenied(types.PermissionDeniedDetails_AGENT_MUST_REATTEST, "agent can't renew SVID, must reattest")
if attestedNode.CanReattest {
log.Warn("Agent renewing SVID when it should be reattesting")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.8.x agents calling this API will always trigger this warning unless they have the feature flag enabled.
It doesn't seem that we can safely enforce reattestation until 1.10 without breaking existing agents.
We should probably just add a TODO comment here to change this to return an error in v1.10.

Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @faisal-memon for your patience on the review. I have one more comment about documenting the setting being introduced.

@@ -103,6 +103,9 @@ type agentConfig struct {
Experimental experimentalConfig `hcl:"experimental"`

UnusedKeyPositions map[string][]token.Pos `hcl:",unusedKeyPositions"`

// Deprecated configurables
DisableReattestToRenew bool `hcl:"disable_reattest_to_renew"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have this documented in spire_agent.md and agent_full.conf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amartinezfayo docs updated.

Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @faisal-memon!

@amartinezfayo
Copy link
Member

Hey @faisal-memon DCO check is failing:

Commit sha: [fb1b954](https://github.com/spiffe/spire/pull/4791/commits/fb1b954d3b58a1d3f0b9967ce8179ff46798703f), Author: Faisal Memon, Committer: Faisal Memon; The sign-off is missing.

Could you please address that?

faisal-memon and others added 15 commits February 7, 2024 11:13
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/cloudflare/circl](https://github.com/cloudflare/circl) from 1.3.5 to 1.3.7.
- [Release notes](https://github.com/cloudflare/circl/releases)
- [Commits](cloudflare/circl@v1.3.5...v1.3.7)

---
updated-dependencies:
- dependency-name: github.com/cloudflare/circl
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.19.0 to 0.20.0.
- [Commits](golang/net@v0.19.0...v0.20.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Cf. "crane digest docker/dockerfile:1.6.0" vs "crane digest --platform linux/amd64 docker/dockerfile:1.6.0"

Signed-off-by: Jürgen Kreileder <jk@blackdown.de>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [actions/cache](https://github.com/actions/cache) from 3.3.2 to 4.0.0.
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@704facf...13aacd8)

---
updated-dependencies:
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/lestrrat-go/jwx/v2](https://github.com/lestrrat-go/jwx) from 2.0.18 to 2.0.19.
- [Release notes](https://github.com/lestrrat-go/jwx/releases)
- [Changelog](https://github.com/lestrrat-go/jwx/blob/develop/v2/Changes)
- [Commits](lestrrat-go/jwx@v2.0.18...v2.0.19)

---
updated-dependencies:
- dependency-name: github.com/lestrrat-go/jwx/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.0.0 to 4.1.0.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@c7d193f...1eb3cb2)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@f44cd7b...6b208ae)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
)

Bumps [github.com/open-policy-agent/opa](https://github.com/open-policy-agent/opa) from 0.59.0 to 0.60.0.
- [Release notes](https://github.com/open-policy-agent/opa/releases)
- [Changelog](https://github.com/open-policy-agent/opa/blob/main/CHANGELOG.md)
- [Commits](open-policy-agent/opa@v0.59.0...v0.60.0)

---
updated-dependencies:
- dependency-name: github.com/open-policy-agent/opa
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
This test is racy because it currently relies on timing of several
goroutines and context cancellation.

The notifier doing context cancellation doesn't seem to test anything
useful. I don't recall why it was even added.

Getting rid of the context cancellation and simply asserting that the
function returns nil when the notifier returns nil seems good enough.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
* Fix racy tests that test streaming RPCs

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
* peertracker: close connection when IsAlive fails

The listener now wraps the platform-specific watcher with one that
closes the underlying connection when the IsAlive check fails in order
to kill the transport to the caller, who is no longer trusted.

Fixes: spiffe#4665

Signed-off-by: Andrew Harding <azdagron@gmail.com>

* relax error check

Signed-off-by: Andrew Harding <azdagron@gmail.com>

---------

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
dependabot bot and others added 20 commits February 7, 2024 11:13
)

Bumps [github.com/open-policy-agent/opa](https://github.com/open-policy-agent/opa) from 0.60.0 to 0.61.0.
- [Release notes](https://github.com/open-policy-agent/opa/releases)
- [Changelog](https://github.com/open-policy-agent/opa/blob/main/CHANGELOG.md)
- [Commits](open-policy-agent/opa@v0.60.0...v0.61.0)

---
updated-dependencies:
- dependency-name: github.com/open-policy-agent/opa
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/hashicorp/vault/api](https://github.com/hashicorp/vault) from 1.10.0 to 1.11.0.
- [Release notes](https://github.com/hashicorp/vault/releases)
- [Changelog](https://github.com/hashicorp/vault/blob/main/CHANGELOG.md)
- [Commits](hashicorp/vault@v1.10.0...v1.11.0)

---
updated-dependencies:
- dependency-name: github.com/hashicorp/vault/api
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
…mpatible (spiffe#4833)

* Bump github.com/docker/docker

Bumps [github.com/docker/docker](https://github.com/docker/docker) from 24.0.7+incompatible to 25.0.1+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Commits](moby/moby@v24.0.7...v25.0.1)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

* update unit test

Signed-off-by: Andrew Harding <azdagron@gmail.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Co-authored-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps the aws-sdk group with 1 update: [github.com/aws/aws-sdk-go-v2/service/ec2](https://github.com/aws/aws-sdk-go-v2).

Updates `github.com/aws/aws-sdk-go-v2/service/ec2` from 1.145.0 to 1.146.0
- [Release notes](https://github.com/aws/aws-sdk-go-v2/releases)
- [Commits](aws/aws-sdk-go-v2@service/ec2/v1.145.0...service/ec2/v1.146.0)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go-v2/service/ec2
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: aws-sdk
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
* Add pagination support to events based cache

Signed-off-by: Faisal Memon <fymemon@yahoo.com>
…iffe#4849)

Bumps [github.com/google/go-containerregistry](https://github.com/google/go-containerregistry) from 0.18.0 to 0.19.0.
- [Release notes](https://github.com/google/go-containerregistry/releases)
- [Changelog](https://github.com/google/go-containerregistry/blob/main/.goreleaser.yml)
- [Commits](google/go-containerregistry@v0.18.0...v0.19.0)

---
updated-dependencies:
- dependency-name: github.com/google/go-containerregistry
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.159.0 to 0.161.0.
- [Release notes](https://github.com/googleapis/google-api-go-client/releases)
- [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md)
- [Commits](googleapis/google-api-go-client@v0.159.0...v0.161.0)

---
updated-dependencies:
- dependency-name: google.golang.org/api
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/mattn/go-sqlite3](https://github.com/mattn/go-sqlite3) from 1.14.20 to 1.14.21.
- [Release notes](https://github.com/mattn/go-sqlite3/releases)
- [Commits](mattn/go-sqlite3@v1.14.20...v1.14.21)

---
updated-dependencies:
- dependency-name: github.com/mattn/go-sqlite3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
…#4852)

* LRU subscribers failed to start when no selector was provided

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [sigstore/cosign-installer](https://github.com/sigstore/cosign-installer) from 3.3.0 to 3.4.0.
- [Release notes](https://github.com/sigstore/cosign-installer/releases)
- [Commits](sigstore/cosign-installer@9614fae...e1523de)

---
updated-dependencies:
- dependency-name: sigstore/cosign-installer
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/shirou/gopsutil/v3](https://github.com/shirou/gopsutil) from 3.23.12 to 3.24.1.
- [Release notes](https://github.com/shirou/gopsutil/releases)
- [Commits](shirou/gopsutil@v3.23.12...v3.24.1)

---
updated-dependencies:
- dependency-name: github.com/shirou/gopsutil/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
…estor (spiffe#4841)

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/docker/docker](https://github.com/docker/docker) from 25.0.1+incompatible to 25.0.2+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Commits](moby/moby@v25.0.1...v25.0.2)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
* Set a default admin socket path for spire-agent cli use

Set a standard default location the spire-agent cli
will use to look for the agent socket when using commands against
the spire-agent daemon. Actual functionality needing this variable
will come in future patches.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

* Incorperate feedback

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

* Fix filename

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

---------

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/mattn/go-sqlite3](https://github.com/mattn/go-sqlite3) from 1.14.21 to 1.14.22.
- [Release notes](https://github.com/mattn/go-sqlite3/releases)
- [Commits](mattn/go-sqlite3@v1.14.21...v1.14.22)

---
updated-dependencies:
- dependency-name: github.com/mattn/go-sqlite3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
…4868)

For consistency sake, the initial state should be loaded from the main
database instance and not from read replicas.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [github.com/sigstore/rekor](https://github.com/sigstore/rekor) from 1.3.4 to 1.3.5.
- [Release notes](https://github.com/sigstore/rekor/releases)
- [Changelog](https://github.com/sigstore/rekor/blob/main/CHANGELOG.md)
- [Commits](sigstore/rekor@v1.3.4...v1.3.5)

---
updated-dependencies:
- dependency-name: github.com/sigstore/rekor
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps the azure-sdk group with 1 update: [github.com/Azure/azure-sdk-for-go/sdk/azcore](https://github.com/Azure/azure-sdk-for-go).

Updates `github.com/Azure/azure-sdk-for-go/sdk/azcore` from 1.9.1 to 1.9.2
- [Release notes](https://github.com/Azure/azure-sdk-for-go/releases)
- [Changelog](https://github.com/Azure/azure-sdk-for-go/blob/main/documentation/release.md)
- [Commits](Azure/azure-sdk-for-go@sdk/azcore/v1.9.1...sdk/azcore/v1.9.2)

---
updated-dependencies:
- dependency-name: github.com/Azure/azure-sdk-for-go/sdk/azcore
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: azure-sdk
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.161.0 to 0.162.0.
- [Release notes](https://github.com/googleapis/google-api-go-client/releases)
- [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md)
- [Commits](googleapis/google-api-go-client@v0.161.0...v0.162.0)

---
updated-dependencies:
- dependency-name: google.golang.org/api
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
* No longer emit x509UniqueIdentifier in X509-SVIDs

Introduced in 1.4.2, this practice has turned out to be problematic.
This change updates SPIRE Server to no long emit attribute in the
X509-SVID subject.

It also introduces a new built-in CredentialComposer to add the
attribute back in for deployments that rely on it. The plugin only
augments workload X509-SVIDs. Server and agent X509-SVIDs are not
modified.

Fixes: spiffe#4755
Fixes: spiffe#3110

Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
@faisal-memon
Copy link
Contributor Author

Hey @faisal-memon DCO check is failing:

Commit sha: [fb1b954](https://github.com/spiffe/spire/pull/4791/commits/fb1b954d3b58a1d3f0b9967ce8179ff46798703f), Author: Faisal Memon, Committer: Faisal Memon; The sign-off is missing.

Could you please address that?

@amartinezfayo done

@amartinezfayo amartinezfayo merged commit 9b09e0f into spiffe:main Feb 7, 2024
32 checks passed
faisal-memon added a commit to faisal-memon/spire that referenced this pull request Feb 9, 2024
* Make reattest to renew default behaviour

Signed-off-by: Faisal Memon <fymemon@yahoo.com>
sriyer pushed a commit to spire-vault/spire that referenced this pull request Feb 23, 2024
* Make reattest to renew default behaviour

Signed-off-by: Faisal Memon <fymemon@yahoo.com>
rushi47 pushed a commit to rushi47/spire that referenced this pull request Apr 11, 2024
* Make reattest to renew default behaviour

Signed-off-by: Faisal Memon <fymemon@yahoo.com>
@faisal-memon faisal-memon deleted the reattest branch July 4, 2024 00:00
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.

spire-agent: move "reattest_to_renew" to the main configuration
10 participants