Skip to content

Authentication of a CID #141

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

Open
silverpill opened this issue Jan 11, 2025 · 11 comments
Open

Authentication of a CID #141

silverpill opened this issue Jan 11, 2025 · 11 comments
Assignees
Labels
editorial This item is editorial in nature. future This item has been deferred to a future version.

Comments

@silverpill
Copy link

In the subjects section:

Dereferencing the canonical URL MUST return the current authoritative controlled identifier document. The returned document's base identifier MUST be the same as the canonical URL; if it is anything else, then the returned document is not an authoritative controlled identifier document and the identifier SHOULD be treated as invalid.

"the identifier SHOULD be treated as invalid" sounds ambiguous. The base identifier of a retrieved document may be valid (we can retrieve it again and may get a matching CID). In this case only canonical URL is invalid.

Also, I think this SHOULD need to be replaced with a MUST. If the base identifier doesn't match the canonical URL (especially if they have different web origins), that may be an impersonation attempt.

@msporny msporny added discuss normative This item is a normative change. labels Jan 12, 2025
@msporny msporny self-assigned this Jan 12, 2025
@msporny
Copy link
Member

msporny commented Jan 12, 2025

The base identifier of a retrieved document may be valid (we can retrieve it again and may get a matching CID). In this case only canonical URL is invalid.

Note that the language says that "dereferencing the canonical URL" has to result in a document with the canonical URL in there. This was done to ensure that a redirect is possible, and a re-attempt (as you mention) can be performed. However, once that re-attempt is made for what is believed to be the canonical URL, that final check MUST succeed. That is, the second you stop getting redirect hints through whatever protocol you're using (like 301 redirects), and once you believe you're at the final document, you need to ensure you're dealing w/ a canonical URL.

Also, I think this SHOULD need to be replaced with a MUST. If the base identifier doesn't match the canonical URL (especially if they have different web origins), that may be an impersonation attempt.

We considered that, but felt like the spec is too new to do that sort of hard enforcement and that we wanted to depend on implementer experience over the next couple of years before we lock it down. There might be legitimate reasons to not throw an error (like you know the document is out of date, but you've deemed that to be ok for some reason).

All this said, I don't think we'd change the normative text at this point for the reason above.

Can you suggest some text that might address your concern?

@msporny msporny added editorial This item is editorial in nature. and removed normative This item is a normative change. labels Jan 12, 2025
@silverpill
Copy link
Author

That is, the second you stop getting redirect hints through whatever protocol you're using (like 301 redirects), and once you believe you're at the final document, you need to ensure you're dealing w/ a canonical URL.

Thank you for clarification. I think the bit about redirects is important to get right and additional text would help implementers understand this requirement:

The returned document's base identifier MUST be the same as the canonical URL (the final URL in the chain of redirects);

However, I still don't understand what "the identifier" refers to at the end of the sentence:

if it is anything else, then the returned document is not an authoritative controlled identifier document and the identifier SHOULD be treated as invalid.

Perhaps it was meant to be "and the document SHOULD be treated as invalid", or "and the base identifier SHOULD be treated as invalid"?

We considered that, but felt like the spec is too new to do that sort of hard enforcement and that we wanted to depend on implementer experience over the next couple of years before we lock it down.

ActivityPub protocol is very similar in this regard. AP documents have an id property, and when document URL is dereferenced, software must verify that the final location in the chain of redirects matches the document id. Failure to do so leads to severe vulnerabilities, see GHSA-3fjr-858r-92rw for example.

There might be legitimate reasons to not throw an error (like you know the document is out of date, but you've deemed that to be ok for some reason).

My experience with ActivityPub tells me that such legitimate reasons don't exist, but if they do exist, they should probably be listed.

@msporny msporny added ready for pr This issue is ready to have a pull request created for it. and removed discuss labels Jan 22, 2025
@selfissued
Copy link
Collaborator

selfissued commented Jan 22, 2025

Since this is a security validation process, the current "MUST" is appropriate. I'm OK adding additional text describing conditions when the MUST might not be satisfied, but I'm not OK for the document to be considered valid when it is not.

@iherman
Copy link
Member

iherman commented Jan 22, 2025

The issue was discussed in a meeting on 2025-01-22

  • no resolutions were taken
View the transcript

2.2. Authentication of a CID (issue cid#141)

See github issue cid#141.

Ivan Herman: the next is on CID.
… manu, this is probably another one for you.

Manu Sporny: this issue is related to fediverse and ActivityPub use.
… JoeAndrieu you added something about base identifier and canonical identifier.
… when you get a CID value back it has an id value. That is supposed to match the URL you got the document from.
… we say that if that is not the case, then the document should be treated as invalid.
… the request is to make that a MUST.
… we did not do that earlier because there may be cases where it is valid for them not to match.
… such as the document being retrieved from cache even when the document is gone.
… or if query parameters, etc.
… so we avoided MUST language and used SHOULD language.
… so a further suggestion was to describe those cases.
… for the group: do we want to make this a MUST?
… if not, do we want to describe cases where these may not match?

Michael Jones: this is a security validation feature, so those should be MUSTs.
… we could informatively describe where that MUST would not be enforced.
… or we could say MUST...UNLESS...

Dave Longley: I think that MUST...UNLESS... construct is what is in there now, actually...just with different words.
… it is conditional.
… in the unless case we say you should treat it as invalid.

Michael Jones: that sounds like it's already addressed then.

Manu Sporny: I agree, this is already addressed.
… do we want to describe the scenarios where it might be OK for them not to match?
… like trusting a cache?

Ivan Herman: is it a major amount of work?

Manu Sporny: it's about a paragraph.

Ivan Herman: well in that case!!

Manu Sporny: I'll raise that PR.

Ivan Herman: and everyone will be absolutely happy.

@iherman
Copy link
Member

iherman commented Feb 5, 2025

The issue was discussed in a meeting on 2025-02-06

  • no resolutions were taken
View the transcript

4.2. Authentication of a CID (issue cid#141)

See github issue cid#141.

Brent Zundel: might just be a small editorial change.

Manu Sporny: you're just waiting on me to raise a PR.

Brent Zundel: that's it. the rest are tracking issues.

@msporny
Copy link
Member

msporny commented Feb 9, 2025

PR #149 has been raised to address this issue. This issue will be closed once PR #149 has been merged.

@msporny msporny added pr exists A Pull Request exists to address this issue. and removed ready for pr This issue is ready to have a pull request created for it. labels Feb 9, 2025
@brentzundel brentzundel added future This item has been deferred to a future version. and removed pr exists A Pull Request exists to address this issue. labels Feb 19, 2025
@jandrieu
Copy link
Contributor

I think the problem is that the identifier SHOULD be treated as invalid. Yet the document is NOT a valid controlled identifier document.

There's a point that the identifier might in fact, be valid, but not currently resolvable.

@dlongley
Copy link
Contributor

I think the problem is that the identifier SHOULD be treated as invalid. Yet the document is NOT a valid controlled identifier document.

I agree with @jandrieu and the fix should just be to remove the rest of the sentence, so this:

then the returned document is not an authoritative controlled identifier document and the identifier SHOULD be treated as invalid.

Becomes this:

then the returned document is not an authoritative controlled identifier document.

@iherman
Copy link
Member

iherman commented Feb 19, 2025

The issue was discussed in a meeting on 2025-02-19

  • no resolutions were taken
View the transcript

1.4. Controlled Identifiers.

Ivan Herman: implementation report is incomplete.

See github pull request cid#149.

Manu Sporny: nobody likes the wording of this PR yet. Stronger MUST statements are requested, but it is already a MUST, just needs language tweaks. I will take another stab at this when I have time.

Brent Zundel: if this PR is not resolved during the next week then the issue 141 will be marked for future resolution.

Michael Jones: why not simply change SHOULD of existing text to MUST without any rewording.

Manu Sporny: we will also need tests for this.
… we were trying to not introduce new normative statements and more tests.

Michael Jones: I just checked. The CID spec still says "SHOULD be treated as invalid".

Manu Sporny: we could instead simply delete the statement because currently it is not being tested.

Michael Jones: I'm also fine marking this as future or pending close.

Brent Zundel: any opposition of marking this for future now?

Manu Sporny: +1 to mark as future, we need more time to find the right text.

Ivan Herman: +1 to future.

Dave Longley: +1 to mark it as future, simplest fix is to delete the text starting with "and the identifier".

Joe Andrieu: +1.

See github issue cid#141.

Manu Sporny: can we close some issues prior to the review i.e. 22, 23, 25.

Michael Jones: I'm good.

@silverpill
Copy link
Author

+1 to deleting "and the identifier SHOULD be treated as invalid"

@jandrieu
Copy link
Contributor

+1 to the deletion proposal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial This item is editorial in nature. future This item has been deferred to a future version.
Projects
None yet
Development

No branches or pull requests

7 participants