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

Enforce OAuth #7

Closed
wants to merge 5 commits into from
Closed

Enforce OAuth #7

wants to merge 5 commits into from

Conversation

swthorn
Copy link

@swthorn swthorn commented Sep 13, 2022

Hi,

I have modified all Network Functions to properly request and validate OAuth tokens from the NRF's authorization server. This will be part of a larger pull request to free5GC as there are a few repositories to which I need to make pull requests.

This pull request modifies the OpenAPI helper libraries that most NFs use. It correctly requests OAuth access tokens from the NRF and sends them along with all requests. This PR looks quite big because most files are changed, however, it is relatively easy to change the code as it's automatically generated and inserted by another program. Please let me know if you have any improvements you want me to make.

NRF Pull Request: free5gc/nrf#8
NSSF Pull Request: free5gc/nssf#5

When these three PR's are merged, I will create other pull requests for the rest of the Network Functions. Please let me know if you have further questions.

Thanks!

This was referenced Sep 13, 2022
@tim-ywliu
Copy link
Collaborator

@swthorn

This PR will cause requesting OAuth access token too frequently. Once getting the access token, it can be reused for the service next time.

@swthorn
Copy link
Author

swthorn commented Oct 17, 2022

@tim-ywliu

Yes, I agree. This implementation has a trade-off between performance and security by adding in OAuth. For these reasons, I add a configuration option to control OAuth enforcement in each NF PR. e.g: https://github.com/free5gc/nssf/blob/b6db32363b421143bab86102a79673ae6a507b27/internal/context/context.go#L55

I agree that there should be a more sophisticated caching approach, but as an initial implementation of OAuth, let me know your thoughts on the current code.

@swthorn
Copy link
Author

swthorn commented Oct 20, 2022

Hi,

I have a couple of quick notes on the merge conflicts on this PR.

return strings.Contains(scope, serviceName)
I think this is not sophisticated enough to verify scope. It may allow for subtle attacks you're not expecting. I believe my function here: https://github.com/swthorn/openapi-1/blob/f8fa664f62b7d84bd17dfe7759ae3e5ca01f9694/oauth.go#L54 does a better job at verifying scopes in the JWT.

Secondly,

token, err := jwt.ParseWithClaims(
does not verify the algorithm in the JWT. The JWT library states this is a security flaw here. Here's a writeup on how to perform an attack: https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

Finally, I see the TLS certificates are re-generated in the code base (for each run?). Just clarifying that this is intended behavior? Is this a realistic expectation in a deployed telco core?

@tim-ywliu
Copy link
Collaborator

@swthorn

Thanks for your report and we'll check the scope & JWT verification.

About certificates generation, the design is that NRF will generate root cert/key if they are not available and all NFs will generate its own cert/key if they are not available.
So, you can also pre-generate the cert/key and NRF/NF will directly use them from the path defined in config.

PS: root cert & NF's certs should be put in the same folder. Keys can be put in different path which only NF can access by itself.

@swthorn
Copy link
Author

swthorn commented Nov 14, 2022

Hi, just following up on this. Is there something I can do to speed up the review process? Is the hold-up because of the performance trade-off for not caching access tokens?

@eggegg31415 eggegg31415 mentioned this pull request Nov 14, 2022
@swthorn
Copy link
Author

swthorn commented Apr 13, 2023

Any updates on the status of this PR?

@tim-ywliu
Copy link
Collaborator

Sorry, currently we are busy new releasing, so it will be pending util the new release done.

@swthorn
Copy link
Author

swthorn commented Dec 15, 2023

Hi,

I noticed you guys merged PRs in other NFs but have not updated this yet. I'm sure you know that you will need to update this repo too. Please let me know how I can assist in this process.

Specifically, now that the underlying OAuth method in the NFs has changed significantly since when I made initial PRs (I haven't reviewed the changes you decided to make), how do you want me to adapt the automated instrumentation of the APIs in this repository? It is trivial for me, and a lot of work for you to do manually.

Thanks!

@tim-ywliu
Copy link
Collaborator

@swthorn

We use #19 instead and will close this PR, thanks.

@tim-ywliu tim-ywliu closed this Dec 19, 2023
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.

2 participants