Skip to content

fix: warn operators about deprecated attributes in session requests (#522) - #579

Open
dobby-coder[bot] wants to merge 4 commits into
masterfrom
fix/warn-deprecated-attributes-522
Open

fix: warn operators about deprecated attributes in session requests (#522)#579
dobby-coder[bot] wants to merge 4 commits into
masterfrom
fix/warn-deprecated-attributes-522

Conversation

@dobby-coder

@dobby-coder dobby-coder Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Closes #522

Problem

When a disclosure session requests an attribute whose credential type or issuer is deprecated (e.g. pbdf.pbdf.mobilenumber.mobilenumber), irmago returned only a generic end-user error and logged nothing server-side — even with verbose logging enabled. Operators had no way to tell why the session could not be completed.

Repro (from the issue)

irma session --server http://localhost:8088 --disclose pbdf.pbdf.mobilenumber.mobilenumber

Expected: a clear error/warning saying the attribute is deprecated. Actual: silent failure, generic error in the app.

Fix

  • Add AttributeConDisCon.DeprecatedAttributes(conf) (irma/requests.go), which returns the deduplicated requested attribute identifiers whose credential type or issuer is deprecated (DeprecatedSince set and in the past), mirroring the existing deprecation check in irmaclient.

  • The irma server now logs a clear warning at session-request validation time (validateRequest in irma/server/irmaserver/helpers.go) naming the deprecated attribute identifier(s):

    Session request contains deprecated attribute(s) which clients are generally unable to disclose: pbdf.pbdf.mobilenumber.mobilenumber

Why a warning, not a hard error

A user who still holds a non-expired instance of a deprecated credential can legitimately keep disclosing it (credCandidates/satisfiesCon do not filter held credentials on deprecation). Rejecting the request outright would break those still-valid disclosures, so the request is left to proceed while the operator gets clear visibility in the logs.

Tests

TestDeprecatedAttributes in irma/irmago_test.go covers:

  • no deprecated attributes → empty result
  • deprecated credential type is reported
  • deprecated issuer is reported
  • future deprecation date is not reported
  • each deprecated attribute reported at most once (dedup)

Runs without docker:

go test ./irma/ -run TestDeprecatedAttributes -v   # PASS
go build ./irma/ ./irma/server/irmaserver/         # clean
go vet   ./irma/ ./irma/server/irmaserver/         # clean
gofmt -l (changed files)                           # clean

The full server/keyshare suites require docker services and are left to CI.

🤖 Generated with Claude Code

…522)

When a disclosure session requested an attribute whose credential type or
issuer is deprecated (e.g. pbdf.pbdf.mobilenumber.mobilenumber), irmago
returned only a generic end-user error and logged nothing, even with verbose
logging enabled. This made it hard for operators to understand why the
session could not be completed.

Add AttributeConDisCon.DeprecatedAttributes(conf), which returns the
(deduplicated) requested attribute identifiers whose credential type or issuer
is deprecated. The irma server now logs a clear warning at session-request
validation time naming those attribute identifiers.

The warning is non-blocking: a user who still holds a non-expired instance of
a deprecated credential can keep disclosing it, so the request is not rejected.

Add a regression test covering deprecated credential types, deprecated issuers,
future deprecation dates and deduplication.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rubenhensen
rubenhensen marked this pull request as ready for review June 26, 2026 12:23
@rubenhensen

Copy link
Copy Markdown
Contributor

/dobby fix the failing CI checks

@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

On it, @rubenhensen! I'm picking up the CI fixes for irmago#579 and will open a draft PR with the changes shortly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Dobby's run crashed before it finished and could not be recovered automatically: Rescue could not continue: the crashed node is unknown.

If this still needs doing, ping Dobby again.

@rubenhensen

Copy link
Copy Markdown
Contributor

/dobby would this work with eudi cert based or diip DID based attributes?

@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

On it! I'm looking into the EUDI certificate and DIIP DID attribute compatibility question now.

@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Good question — and the short answer is no, not as written. This PR only covers the classic Yivi/IRMA disclosure path, not the EUDI cert-based or DIIP DID-based (OpenID4VP) flows. Here's why, after tracing the code:

What the PR actually hooks into

The new check lives in the verifier server's validateRequest (irma/server/irmaserver/helpers.go) and runs over request.Disclosure().Disclose — the IRMA AttributeConDisCon. DeprecatedAttributes (irma/requests.go) then reads the deprecation signal from the IRMA scheme model:

credType := conf.CredentialTypes[attr.Type.CredentialTypeIdentifier()]
issuer   := conf.Issuers[credType.IssuerIdentifier()]
// credType.DeprecatedSince / issuer.DeprecatedSince

So it's entirely scoped to (a) the classic IRMA condiscon request format and (b) DeprecatedSince on irma.CredentialType / irma.Issuer (irma/descriptions.go).

EUDI cert-based (X.509) and DIIP DID-based attributes

Those are disclosed over OpenID4VP with DCQL queries, which is a separate validation path:

  • cert-based → eudi/openid4vp/x509_verifier_validator.go
  • DID-based → eudi/openid4vp/did_verifier_validator.go

Both go through ParseAndVerifyAuthorizationRequest and validate DCQL credential queries (scheme.CredentialQueryInfo) against the relying party's authorized attribute sets — they never construct an IRMA Disclose condiscon and never call irmaserver.validateRequest. So DeprecatedAttributes is simply not on that code path, and the warning would never fire for an EUDI/DIIP session.

On top of that, the EUDI scheme has no deprecation concept at all: grep -r "Deprecat" eudi/ returns nothing. DeprecatedSince only exists on the classic IRMA scheme descriptions.

The one nuance

SD-JWT VC credentials that are issued over IRMA are bridged into irma.Configuration.CredentialTypes (the DCQL handler maps vctirma.NewCredentialTypeIdentifier(vct) and looks it up in config.CredentialTypes — see eudi/openid4vp/irma_sdjwt_dcql/handler.go). So such a credential type structurally carries a DeprecatedSince field. That means if one of those is requested through a classic IRMA disclosure request, this PR's check would correctly flag it. But that's the IRMA path again — the cert-based / DID-based OpenID4VP disclosure you're asking about is not covered.

To extend it to EUDI/DIIP you'd need two things this PR doesn't attempt:

  1. A deprecation concept in the EUDI scheme model (there's none today), and
  2. An equivalent warning in the OpenID4VP/DCQL validation path (x509_verifier_validator.go / did_verifier_validator.go), since those never touch validateRequest.

Happy to open a follow-up issue to track EUDI/DIIP deprecation warnings if that's wanted — but I'd keep it out of this PR, which is a focused fix for #522 (classic IRMA disclosure).

@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Good question, @rubenhensen! I dug into the code and the short answer is no — PR #579 only covers the classic IRMA disclosure path.

The deprecation check hooks into irmaserver.validateRequest which processes IRMA condiscon-style requests. EUDI cert-based (X.509) and DIIP DID-based flows use a separate OpenID4VP+DCQL validation path (eudi/openid4vp/{x509,did}_verifier_validator.go) that never calls validateRequest, and the EUDI scheme has no deprecation concept at all.

One nuance worth noting: SD-JWT VC credentials issued over IRMA are mapped into config.CredentialTypes, so they'd be flagged if requested via a classic IRMA condiscon — but not via OpenID4VP.

Left a full breakdown in the PR comment: #579 (comment) — and I'd suggest opening a follow-up issue to tackle deprecation handling for the EUDI/DIIP paths separately, keeping this fix focused.

@dobby-coder

dobby-coder Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Dobby's run crashed before it finished and could not be recovered automatically: Rescue could not continue: the crashed node is unknown.

If this still needs doing, ping Dobby again.

@rubenhensen
rubenhensen self-requested a review July 22, 2026 06:28
@rubenhensen

Copy link
Copy Markdown
Contributor

/dobby fix conflicts

@dobby-coder

dobby-coder Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Got it — I'm on it. I'll take a look at PR #579 and work on resolving the merge conflicts.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

VERDICT: approve

Conflicts resolved and the branch is now mergeable against master. Consolidated gatekeeper review: clean, no blocking issues.

Reviewed HEAD 4917538:

  • Focused Go change (4 files, 146 lines): new AttributeConDisCon.DeprecatedAttributes(conf) in irma/requests.go + a warning at validateRequest in irma/server/irmaserver/helpers.go.
  • strings import present after the merge — build stays clean.
  • No struct ==/!= pointer-equality trap (uses .IsZero()/.Before()).
  • Regression test TestDeprecatedAttributes covers cred-type/issuer deprecation, future-date exclusion, and dedup.
  • Conventional title, Closes #522, CHANGELOG entry present.

Rule sweep (Go/PR/prose rules) and Review Dobby 2 both returned zero findings.

@dobby-coder

dobby-coder Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Conflicts are resolved — the branch merges cleanly against master now. I ran a full check on top of that (rule sweep, review pass, build/import sanity, and confirmed the TestDeprecatedAttributes regression test survived the merge), everything came back clean, and I've signed off with an approve review.

Since this PR is mine, I left it as a COMMENT-type approval rather than self-merging — it's ready whenever a maintainer wants to merge it.

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.

No error/warning when disclosing a deprecated attribute

1 participant