Skip to content

Use Common Name instead of SPIFFE URI for mTLS app identity#1270

Merged
minwoox merged 1 commit intoline:mainfrom
minwoox:useCNforMTLS
Mar 6, 2026
Merged

Use Common Name instead of SPIFFE URI for mTLS app identity#1270
minwoox merged 1 commit intoline:mainfrom
minwoox:useCNforMTLS

Conversation

@minwoox
Copy link
Contributor

@minwoox minwoox commented Mar 6, 2026

Motivation:

Using the X.509 Subject CN (Common Name) is a more straightforward and broadly compatible approach for certificate-based identity.

Modifications:

  • Renamed SpiffeIdExtractor to CommonNameExtractor and rewrote the extraction logic to parse the certificate.

Result:

  • mTLS-based application authentication identifies clients by the CN field of the certificate.

… app identity

Motivation:

Using the X.509 Subject CN (Common Name) is a more straightforward and broadly compatible approach for certificate-based identity.

Modifications:
- Renamed `SpiffeIdExtractor` to `CommonNameExtractor` and rewrote the extraction logic to parse the certificate.

Result:
- mTLS-based application authentication identifies clients by the CN field of the certificate.
@minwoox minwoox added this to the 0.81.0 milestone Mar 6, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

This PR refactors certificate ID extraction from SPIFFE URI in Subject Alternative Names to Common Name in certificate Subject DN. It updates the core extractor implementation, renames the class accordingly, modifies test fixtures to match the new extraction approach, and updates UI help text and method visibility.

Changes

Cohort / File(s) Summary
Certificate ID Extraction Refactoring
server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/CommonNameExtractor.java, server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java
Renamed SpiffeIdExtractor to CommonNameExtractor and replaced SAN-based SPIFFE URI extraction with LDAP DN CN parsing. Updated the authorizer to reference the new extractor instance. Exception handling changed from CertificateParsingException to InvalidNameException.
Test Fixture Updates
server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/CertificateAppIdentityAuthTest.java, server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java
Updated CERT_ID constant values and simplified SignedCertificateExtension construction by removing SPIFFE URI list parameter. Removed @SuppressWarnings annotations in metadata service test.
UI Help Text Updates
webapp/src/dogma/features/app-identity/NewAppIdentity.tsx
Updated FormHelperText descriptions for appId and certificateId fields to clarify expected values and extraction method (CN or SPIFFE ID in SAN).
Build Script Method Visibility
gradle/scripts/lib/java-javadoc.gradle
Changed downloadListFile method visibility from private to public.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 From SPIFFE URIs we make our retreat,
To Common Names, a simpler treat!
With DN parsing, clean and bright,
Certificate extraction done right.
hops contentedly

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: switching from SPIFFE URI to Common Name for mTLS app identity, which aligns with the core modification across all changed files.
Description check ✅ Passed The description provides relevant context about the change, including motivation, modifications, and expected result, all of which relate directly to the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/CommonNameExtractor.java (1)

46-48: Consider using DEBUG level for parsing failures.

While TRACE is appropriate for high-frequency events, an InvalidNameException during DN parsing could indicate a certificate configuration issue worth investigating. Consider using DEBUG level to make it slightly easier to troubleshoot without enabling full tracing.

That said, TRACE is acceptable if you expect this to be rare and want minimal log noise.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/CommonNameExtractor.java`
around lines 46 - 48, Change the logging level in CommonNameExtractor's
InvalidNameException catch block from TRACE to DEBUG: inside the catch
(InvalidNameException e) block replace the logger.trace call with logger.debug
so DN parsing failures are logged at debug level (preserving the exception
parameter) to aid troubleshooting without enabling full tracing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@webapp/src/dogma/features/app-identity/NewAppIdentity.tsx`:
- Around line 175-178: Update the helper text in NewAppIdentity.tsx to
accurately reflect the backend behavior: change the FormHelperText message
(within the NewAppIdentity component) to indicate that only the Common Name (CN)
from the certificate Subject DN is used by the CommonNameExtractor and remove
any mention of SPIFFE IDs or SAN extraction so users won't expect SPIFFE ID
support.

---

Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/CommonNameExtractor.java`:
- Around line 46-48: Change the logging level in CommonNameExtractor's
InvalidNameException catch block from TRACE to DEBUG: inside the catch
(InvalidNameException e) block replace the logger.trace call with logger.debug
so DN parsing failures are logged at debug level (preserving the exception
parameter) to aid troubleshooting without enabling full tracing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0337df10-95b0-4ec1-b3b7-c175e0ccae2f

📥 Commits

Reviewing files that changed from the base of the PR and between c350b8d and cbc4c70.

📒 Files selected for processing (6)
  • gradle/scripts/lib/java-javadoc.gradle
  • server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java
  • server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/CommonNameExtractor.java
  • server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/CertificateAppIdentityAuthTest.java
  • server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java
  • webapp/src/dogma/features/app-identity/NewAppIdentity.tsx

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

👍👍

@minwoox minwoox merged commit f6e3d78 into line:main Mar 6, 2026
13 of 14 checks passed
@minwoox minwoox deleted the useCNforMTLS branch March 6, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants