Skip to content
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

Add RPKI Signed Checklist Auth #30

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

TheEnbyperor
Copy link

The PR adds a definition for how RPKI Signed Checklists can be used to authenticate to a Peering API.

I've also reorganized some parts to fit this in, and made the graphs render as SVGs using aasvg.

Some more general comments on other bits of the draft from reading through it whilst working on it:

The PeeringDB Auth is woefully underspecified, and maybe even broken. I tried to follow it and made a client_credentials application owned by my organization. When I used the access token to query https://auth.peeringdb.com/profile/v1 this returned a 500 Internal Server Error so this doesn't appear to be the correct way to go about things. The draft talks about OAuth Authorization Code Exchange grant type, but that is (to my understanding) for interactive login sessions with a human present. This draft defines a machine to machine API so the Client Credentials grant type should be used instead.

I'm also not a fan of defining the API only in an OpenAPI document. This is fine to have in addition but were this document to end up as an RFC it should be a self contained document that doesn't include a file in GitHub as a core part of its functionality. I'm happy to rewrite the API definition into a format that fits into an I-D.

Finally I think the example API flow could do with rewriting to make it easier to follow. It's also lacking in BCP14 terms so its unclear what is a hard requirement and what is a recommendation.

@jramseyer
Copy link
Collaborator

jramseyer commented Oct 21, 2024

Wow, I'm embarrassed--I was out on vacation and completely missed this PR. I'm sorry for the late review! Thanks for writing it up.

A few questions:

I'm also not a fan of defining the API only in an OpenAPI document. This is fine to have in addition but were this document to end up as an RFC it should be a self contained document that doesn't include a file in GitHub as a core part of its functionality. I'm happy to rewrite the API definition into a format that fits into an I-D.

Thanks for the feedback. We would welcome the rewrite, if it is not in the correct format now. We had attempted to rewrite it here: https://www.ietf.org/archive/id/draft-ramseyer-grow-peering-api-05.html#name-endpoints
but if you can point us to a more "I-D" formatted example, I'm happy to rewrite it/would welcome any PRs to do so.

The PeeringDB Auth is woefully underspecified, and maybe even broken. I tried to follow it and made a client_credentials application owned by my organization. When I used the access token to query https://auth.peeringdb.com/profile/v1 this returned a 500 Internal Server Error so this doesn't appear to be the correct way to go about things. The draft talks about OAuth Authorization Code Exchange grant type, but that is (to my understanding) for interactive login sessions with a human present. This draft defines a machine to machine API so the Client Credentials grant type should be used instead.

Interesting, will take a look. Let's open this one separately as an issue to discuss. [EDIT]: Reported in #32

@jramseyer
Copy link
Collaborator

The draft looks good to me! I'll merge it in on Wednesday, assuming no further comments.
@TheEnbyperor , should we add a reference to https://datatracker.ietf.org/doc/draft-misell-grow-rpki-peering-api-discovery/ as well? Could add it as part of this one, or I can add a reference separately.

Thanks for the pull request!

This was referenced Oct 21, 2024
@TheEnbyperor
Copy link
Author

The draft looks good to me! I'll merge it in on Wednesday, assuming no further comments. @TheEnbyperor , should we add a reference to https://datatracker.ietf.org/doc/draft-misell-grow-rpki-peering-api-discovery/ as well? Could add it as part of this one, or I can add a reference separately.

Yes, that should probably be referenced informatively.

@TheEnbyperor
Copy link
Author

Whoops, one major oversight a friend just pointed out - there was nothing stopping example.com from taking a nonce from example.org and giving it to AS64496 thereby getting access to example.org as AS64496. I've added a binding to the intended origin to fix this.

@job
Copy link
Member

job commented Oct 22, 2024

Whoops, one major oversight a friend just pointed out - there was nothing stopping example.com from taking a nonce from example.org and giving it to AS64496 thereby getting access to example.org as AS64496. I've added a binding to the intended origin to fix this.

are you sure the information actually needs to be inside files over which one hashes?

The relying party was the one that issued the nonce-to-be-signed-over, so it can simply check whether the nonce matches the value in the peering-api-oauth-nonce field.

The relying party knows the peering-api-oauth-origin it expects, so it can simply check whether the sha256 for the URI it expects is contained within the field peering-api-oauth-origin.

All in all, the "filenames" are just labels for SHA256 values

@job
Copy link
Member

job commented Oct 22, 2024

In other words, the names & hashes themselves can be used to communicate between Issuer and RP; no need to open() any files

@TheEnbyperor
Copy link
Author

This is probably poor wording on my part but I never intended the spec to imply fopen(3)ing files.

@job
Copy link
Member

job commented Oct 23, 2024

Perhaps I am misreading the suggested changes, but it seems that an RSC is only requested in 1 direction, right? Why not in both? Both sides of the to-be-established peering relationship have to authenticate each other that they are who they say they are, right? Doing it both ways should help overcome the need to include the origin in the signed payload.

@TheEnbyperor
Copy link
Author

I can add mutual authentication, but I don't think this eliminates the need to include an audience and subject in both RSCs.

@jramseyer
Copy link
Collaborator

I am not an RSC expert, but I like the idea of both sides authenticating. It would give us a better guarantee that both the client and server here are legitimate entities.

From a security standpoint in other auth methods, the server is implicitly trusted by the client. You could argue that you shouldn't request peering if you don't believe that the server is trustworthy, but I like the idea that, with mutual RSC verification, the client can confirm that the server is trustworthy as well.

I defer to you both on the inclusion of the origin/subject in RSCs.

@grizz
Copy link
Member

grizz commented Oct 25, 2024

The PeeringDB Auth is woefully underspecified, and maybe even broken. I tried to follow it and made a client_credentials application owned by my organization. When I used the access token to query https://auth.peeringdb.com/profile/v1 this returned a 500 Internal Server Error so this doesn't appear to be the correct way to go about things. The draft talks about OAuth Authorization Code Exchange grant type, but that is (to my understanding) for interactive login sessions with a human present. This draft defines a machine to machine API so the Client Credentials grant type should be used instead.

@TheEnbyperor Would you mind reporting that to https://github.com/peeringdb/peeringdb/issues? I'm happy to as well if you'd prefer.

@jramseyer
Copy link
Collaborator

@TheEnbyperor @job OK by both of you to merge this in? I'd like to post a new version before the next IETF meeting, if we can manage it.
Thanks!

@job
Copy link
Member

job commented Oct 25, 2024

Let's hold off on merging this - a case has been made that an improved version of RSC is needed to do the Peering API any justice.

@jramseyer
Copy link
Collaborator

@job @TheEnbyperor Thanks for the update, I will wait to merge this PR.

We plan to post a new version to the IETF on Monday. If this one is ready, I'll include it. If not, we can leave this one out and include it in a later version. Thanks!

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.

4 participants