Skip to content

kbs: add insecure options for admin/attestation token verification - #1360

Open
Xynnn007 wants to merge 3 commits into
confidential-containers:mainfrom
Xynnn007:kbs/add-http-key
Open

kbs: add insecure options for admin/attestation token verification#1360
Xynnn007 wants to merge 3 commits into
confidential-containers:mainfrom
Xynnn007:kbs/add-http-key

Conversation

@Xynnn007

@Xynnn007 Xynnn007 commented May 20, 2026

Copy link
Copy Markdown
Member

This patch adds three switches of insecure options for admin/attestation token verification

  • Allow fetch verification public keys from HTTP endpoints for Attestation Token
  • Allow fetch verification public keys from HTTP endpoints for Admin Token
  • Skip jwk field's endorsement check for Admin Token

These are useful in dev cases.

Relies on #1239

@Xynnn007
Xynnn007 force-pushed the kbs/add-http-key branch from bf46a74 to c5242ec Compare May 22, 2026 03:44
@Xynnn007
Xynnn007 marked this pull request as ready for review May 22, 2026 03:45
@Xynnn007
Xynnn007 requested a review from a team as a code owner May 22, 2026 03:45
@Xynnn007
Xynnn007 requested review from fitzthum and mythi May 23, 2026 00:57
Comment thread kbs/docs/admin.md Outdated
@Xynnn007
Xynnn007 force-pushed the kbs/add-http-key branch from c5242ec to 1c37635 Compare May 25, 2026 12:14
@Xynnn007

Copy link
Copy Markdown
Member Author

Added another commit for attestation token part to allow http:// jwk set, which will help to connect to an http .well-known to get jwk sets.

@Xynnn007 Xynnn007 changed the title kbs: add http config to fetch token verification key for admin kbs: add insecure options for admin/attestation token verification May 26, 2026
@Xynnn007
Xynnn007 requested a review from mythi June 12, 2026 02:28
@Xynnn007
Xynnn007 requested a review from Copilot June 16, 2026 11:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds configuration switches to relax JWT verification behavior in KBS for development scenarios, including optionally allowing plaintext http:// key sources and optionally skipping endorsement checks for header-embedded JWKs (admin tokens).

Changes:

  • Add insecure_public_key_from_uri to attestation token verifier config and plumb it into JwtVerifier.
  • Extend JwtVerifier/JWK fetching to support conditional http:// for JWKS/OpenID discovery and PEM public keys.
  • Add admin (bearer_jwt) config flags for insecure key loading and skipping header-JWK endorsement checks, plus docs/tests updates.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
kbs/src/token/mod.rs Adds attestation token config flag and passes it into JwtVerifier.
kbs/src/crypto/jwt.rs Extends verifier construction to thread through the new “allow insecure HTTP” flag for JWKS/PEM loading.
kbs/src/crypto/jwk.rs Updates JWKS loading API to accept the new flag and logs a warning when using HTTP.
kbs/src/config.rs Updates config test fixtures to include the new attestation token field.
kbs/src/attestation/intel_trust_authority/mod.rs Updates JwtVerifier::new call site to include the new parameter.
kbs/src/admin/authentication/bearer_jwt.rs Adds admin auth config flags and tests; wires flags into JwtVerifier::new.
kbs/docs/config.md Documents new attestation/admin configuration options.
kbs/docs/admin.md Documents new bearer_jwt options and supported source formats.
integration-tests/src/common.rs Updates integration test harness config to include the new field.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread kbs/src/crypto/jwk.rs Outdated
Comment thread kbs/src/admin/authentication/bearer_jwt.rs Outdated
Comment thread kbs/docs/admin.md

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread kbs/src/admin/authentication/bearer_jwt.rs
Comment thread kbs/src/crypto/jwk.rs Outdated
Comment thread kbs/src/crypto/jwt.rs
@mythi

mythi commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

My opinion on this PR has not changed. It's an "expensive" developer feature to be added to the config especially when there's no real blockers that justify these changes.

@Xynnn007

Copy link
Copy Markdown
Member Author

My opinion on this PR has not changed. It's an "expensive" developer feature to be added to the config especially when there's no real blockers that justify these changes.

Cannot agree with the point. It's disabled by default and requires explicit user enablement. Just like KBS currently supports both HTTP and HTTPS startup.

In many enterprise environments and development/testing scenarios, where users cannot provide a CA, this can be very helpful. You mentioned using configmaps for configuration, but this requires users to manually update the jwk set instead of automatically fetching updates.

It's important to understand that introducing manual updates is very cumbersome.

@Xynnn007
Xynnn007 force-pushed the kbs/add-http-key branch 2 times, most recently from 0e9230b to a047e9d Compare June 17, 2026 07:35
Comment thread kbs/src/crypto/jwk.rs Fixed
Comment thread kbs/src/crypto/jwt.rs Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment thread kbs/src/crypto/jwk.rs Outdated
Comment thread kbs/src/crypto/jwt.rs
Comment thread kbs/src/crypto/jwk.rs Outdated
Comment thread kbs/src/crypto/jwk.rs Outdated
Comment thread kbs/src/crypto/jwk.rs Outdated
@mythi

mythi commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

My opinion on this PR has not changed. It's an "expensive" developer feature to be added to the config especially when there's no real blockers that justify these changes.

Cannot agree with the point. It's disabled by default and requires explicit user enablement. Just like KBS currently supports both HTTP and HTTPS startup.

What was done in the past does not necessarily mean it needs to be followed.

In many enterprise environments and development/testing scenarios, where users cannot provide a CA, this can be very helpful. You mentioned using configmaps for configuration, but this requires users to manually update the jwk set instead of automatically fetching updates.

It's important to understand that introducing manual updates is very cumbersome.

This PR has changed from admin only topic to something else. My comments have been related to that admin http jwkset part only. How likely it is that these jwksets need to be updated regularly during testing?

Can we keep the two separate PRs so that one topic does not block the other.

@Xynnn007

Copy link
Copy Markdown
Member Author

In many enterprise environments and development/testing scenarios, where users cannot provide a CA, this can be very helpful. You mentioned using configmaps for configuration, but this requires users to manually update the jwk set instead of automatically fetching updates.
It's important to understand that introducing manual updates is very cumbersome.

This PR has changed from admin only topic to something else. My comments have been related to that admin http jwkset part only. How likely it is that these jwksets need to be updated regularly during testing?

Can we keep the two separate PRs so that one topic does not block the other.

Not just testing, but inner deployments

I get attestation-token things into #1434.

About admin part, I believe this follows the industry-standard secure-by-default pattern: Redis protected-mode defaults to yes and only disables when explicitly set to no in config. Similarly, this PR keeps things disabled by default, requiring an explicit configuration change to enable. OWASP Proactive Controls C5 states "the default settings are always the most secure option," and CISA's Secure by Design guidance confirms "out-of-the-box, products should be secure." Defaulting to off and requiring explicit opt-in does not introduce risk—it acknowledges legitimate use cases while ensuring they are intentional and auditable. Removing this option would only push users toward less visible workarounds.

Completely blocking the path is not what we want; a better approach is to provide a door that users can choose to open or close.

Comment thread kbs/src/admin/authentication/bearer_jwt.rs Outdated
Comment thread kbs/src/token/mod.rs Outdated
Comment thread kbs/src/crypto/jwk.rs Fixed
@fitzthum

Copy link
Copy Markdown
Member

Yeah fwiw I am fine with adding this. We should be careful with adding insecure options, but I think this one is reasonable. This is quite a niche use case, but if you're trying to setup a demo or test that uses an IDP, you probably are not going to have HTTP setup. You could always specify the keys directly in this case, but that changes the flow a bit.

Xynnn007 and others added 3 commits July 9, 2026 10:32
Add insecure_public_key_uri to bearer_jwt and gate plaintext HTTP
when loading admin PEM keys and remote JWKS sources.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Wire insecure_public_key_uri through [attestation_token] so
trusted_jwk_sets can be loaded over plaintext HTTP in controlled environments.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add insecure_header_jwk to bearer_jwt to control whether header-embedded
JWK keys must be endorsed via x5c before verifying admin JWT signatures.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@Xynnn007
Xynnn007 force-pushed the kbs/add-http-key branch from 7a0258f to d423a16 Compare July 9, 2026 02:36
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.

6 participants