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

Option to enable authentication for all cargo API routes #176

Open
fsavy-tehtris opened this issue Sep 27, 2023 · 2 comments
Open

Option to enable authentication for all cargo API routes #176

fsavy-tehtris opened this issue Sep 27, 2023 · 2 comments
Assignees
Labels
C-enhancement Category: Enhancement M-api Module: Programmatic API P-medium Priority: Medium

Comments

@fsavy-tehtris
Copy link

With the stabilization of credential-process in cargo, more routes can be protected with a token that cargo can provide. (this is kinda related to #93)

Are there any plans to add authentication to these other routes (I'm mostly interested in download)?

From a quick glance, I believe this could be achieved by adding an Auth parameter like it's done in publish:

pub(crate) async fn put(
State(state): State<Arc<AppState>>,
Auth(author): Auth,

We also probably need an additional configuration value to let users enable or disable this feature. Would you welcome a PR with such modifications?

@Hirevo Hirevo self-assigned this Sep 29, 2023
@Hirevo Hirevo added C-enhancement Category: Enhancement P-medium Priority: Medium M-api Module: Programmatic API labels Sep 29, 2023
@Hirevo
Copy link
Owner

Hirevo commented Sep 29, 2023

Hi, thank you for notifying me about this.

The credential-process RFC is rather specific to Cargo (on the user's machine) and doesn't need any changes on the side of the registries.

But the registry-auth RFC (which has been stabilised alongside credential-process in rust-lang/cargo#12649) does specify how registries can communicate to Cargo to send the Authorization header for every endpoints (like crate search and crate download).

They seem to have done this about two weeks ago and I completely missed it, thanks again for the notification.

As a first approach, I think you are right to propose a new configuration option, similar to the login_required one in [frontend] (maybe a new auth_required option in [general] ?).

Implementation-wise, this would mean to make the previously-unauthenticated endpoints take in an Option<Auth> argument and make the relevant checks in the body, in the same manner that the frontend does it with login_required:

if state.is_login_required() && user.is_none() {
return Ok(Either::E2(Redirect::to("/account/login")));
}

@Hirevo
Copy link
Owner

Hirevo commented Sep 29, 2023

Actually, now that there is a new auth-required option in the crate index's config.json file, one way to implement this could be to have the registry read out this value when starting up, instead of having a new configuration option of our own which could get out of sync with that config.json file.

We could even make the frontend.login_required option be optional and make it inherit from that auth-required option from the crate index if it is omitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement M-api Module: Programmatic API P-medium Priority: Medium
Projects
None yet
Development

No branches or pull requests

2 participants