Skip to content

Conversation

@jcmelati
Copy link
Collaborator

PR to address #199

@jcmelati jcmelati requested a review from Copilot April 16, 2025 10:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • openid-federation-1_0.xml: Language not supported

@teamktown
Copy link
Collaborator

Some observations

  • key rollover events for the anchor
    • Keyrollover is good but butter is to have clear statements (MUST's) that the clients need to fetch or check for changes.
      • otherwise upon key rollover the clients will cease to function as the chain will invalidate due to being signed with a new key
    • Mitigations could be 'sign material/publish with both new and old keys' where clients would walk the N possible keys as equals and if one validates, that satisfies the trust model -- so treat them as a set. I'm unclear if this is a breaking behaviour change. Operationally it adds resilience, flexibility thus increasing reliability during key change events.
    • refresh rates/validity duration is a function of tolerance for both risk and of acceptable velocity of key material circulated.
      • It should be measured in seconds or minutes and not days / hrs
      • Also note that if 'everyone' polls the material at the top of the hour this could create a DDOS effect with many many clients (a flat federation with anchor and 100's/1000's many leaf nodes.
      • Mitigation: cache locally and psuedo random fetch period + attempt to validate locally until expiry threshold is met

Fast, resilient, highly available trust circulation matters to keeping TCO as low as possible for all involved.

@peppelinux peppelinux requested a review from rohe April 23, 2025 14:35
Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

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

I only suggest to reduce the words as my previous comment

Copy link
Member

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

"SHOULD ensure" doesn't sound actionable to me.

The existing wording seems equivalent to yours, but seems clearer to me.

What are you trying to achieve with this rewording?

Co-authored-by: Giuseppe De Marco <demarcog83@gmail.com>
@jcmelati
Copy link
Collaborator Author

@selfissued with the rewording I am trying to avoid any confusion that the phrase "The Trust Anchor SHOULD set an expiration time on its Entity Configuration" brings regarding making it appear as the exp time attribute on the Entity Configuration is optional.

@jcmelati
Copy link
Collaborator Author

@peppelinux merged your correction

Comment on lines 5733 to 5735
A Trust Anchor MUST publish an Entity Configuration about itself. The expiration
time (exp) set on this Entity Configuration SHOULD ensure that federation participants
re-fetch it at reasonable intervals. When a Trust Anchor rolls over its signing keys, it needs to:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A Trust Anchor MUST publish an Entity Configuration about itself. The expiration
time (exp) set on this Entity Configuration SHOULD ensure that federation participants
re-fetch it at reasonable intervals. When a Trust Anchor rolls over its signing keys, it needs to:
A Trust Anchor MUST publish an Entity Configuration about itself.
The expiration time (exp) set on this Entity Configuration should be chosen
such that it ensures that federation participants re-fetch it at reasonable intervals.
When a Trust Anchor rolls over its signing keys, it needs to:

I get it. Hopefully this slightly updated wording fits the bill.

Co-authored-by: Michael B. Jones <michael_b_jones@hotmail.com>
@selfissued
Copy link
Member

@peppelinux can you re-review?

@selfissued selfissued merged commit ed0dbda into openid:main Apr 24, 2025
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.

Clarify requirement for exp time in Trust Anchor Entity Configuration

4 participants