refactor(server)!: cleanup authorization boilerplate

DDtKey commented 9 months ago

Description

fix not-exposed endpoints to return 404 instead of 403 (they literally not intended to be exposed) - the only breaking change, can be extracted to separate PR
keep authz rules cohesive
use HashSet instead of Vec to check tokens
clean up boilerplate code using macro from actix-web-grants to protect endpoints
utilize ErrorHandlers to log unauthorized errors instead of in-place host handling & etc
disallow blank tokens

Motivation and Context

I came across rustypaste and decided to check the code a little & play locally with it. As a result, some of the authorization rules were not obvious to me. For example:

lack of delete_tokens - hides the endpoint
lack of auth_tokens - allows the access (btw, is it intended? because sound like a bug)
disabled endpoints return 403, but from the point of view of security and correctness it is more likely to be 404 - we literally do not expose them

So I decided to do a little refactoring, which I hope you find useful.

Another part of the changes is the introduction of actix-web-grants - this is a small but quite flexible library, with which it will also be easy to change the rules, for example, switch to RBAC/ABAC or something else (actually I'm maintainer of the library, so it's may be subjective)

How Has This Been Tested?

Mostly by current test-coverage, I kept the same behavior, except 403 -> 404

Changelog Entry

### Fixed

- Return `404` for not exposed endpoints instead of `403`
- Disallow blank `delete_tokens` and `auth_tokens`

Types of Changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation (no code change)
[x] Refactor (refactoring production code)
[ ] Other

Checklist:

[x] My code follows the code style of this project.
[ ] I have updated the documentation accordingly.
[x] I have formatted the code with rustfmt.
[x] I checked the lints with clippy.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

DDtKey commented 9 months ago

feel free to close PR if you don't find it useful

orhun commented 9 months ago

Definitely sounds/looks useful! I can review the code tomorrow.

One thing to note here, you can add fixture tests to test the new behavior.

codecov-commenter commented 9 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (adbd67a) 70.23% compared to head (8d35ca8) 70.16%.

Files	Patch %	Lines
src/auth.rs	87.87%	4 Missing :warning:
src/config.rs	87.50%	1 Missing :warning:
src/server.rs	92.30%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #199 +/- ## ========================================== - Coverage 70.23% 70.16% -0.08% ========================================== Files 11 11 Lines 598 610 +12 ========================================== + Hits 420 428 +8 - Misses 178 182 +4 ``` | [Flag](https://app.codecov.io/gh/orhun/rustypaste/pull/199/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z) | Coverage Δ | | |---|---|---| | [unit-tests](https://app.codecov.io/gh/orhun/rustypaste/pull/199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z) | `70.16% <88.88%> (-0.08%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

DDtKey commented 9 months ago

Thanks for the reference!

I took a look at fixtures and think it makes sense to add new fixtures for not-exposed endpoints (list, version or delete-tokens not configured)

Otherwise I have mostly kept the behavior and there is coverage of the main cases.

Except probably one more case, which hasn’t been covered before: if auth-tokens unset/not configured - access is allowed (just to clarify, is this intended?)

There are unit tests ofc, but I’ll add fixtures a bit later for all these cases!

orhun commented 9 months ago

fix not-exposed endpoints to return 404 instead of 403 (they literally not intended to be exposed) - the only breaking change, can be extracted to separate PR

disabled endpoints return 403, but from the point of view of security and correctness it is more likely to be 404 - we literally do not expose them

I think it makes sense. We can mention this in the changelog as breaking.

lack of delete_tokens - hides the endpoint

The intention was to require tokens to delete files on the server. Do you think we should change that?

lack of auth_tokens - allows the access (btw, is it intended? because sound like a bug) Except probably one more case, which hasn’t been covered before: if auth-tokens unset/not configured - access is allowed (just to clarify, is this intended?)

Yeah, this is intended. Do you think we should have an empty list of tokens for allowing access? It is not a big deal imo.

Another part of the changes is the introduction of actix-web-grants

I saw a derive macro on the endpoints that we want to protect but can you tell me how that works? Also it would be nice to get some information in terms of which version of Actix do you support, what are the plans for the future (will there be any breaking changes) etc.

I took a look at fixtures and think it makes sense to add new fixtures for not-exposed endpoints (list, version or delete-tokens not configured) There are unit tests ofc, but I’ll add fixtures a bit later for all these cases!

Great! Just let me know if you have problems / need guidance while adding them.

DDtKey commented 9 months ago

lack of delete_tokens - hides the endpoint

The intention was to require tokens to delete files on the server. Do you think we should change that?

Not sure, the main confusion is that we have expose flag for list & version - which is really clear, but in case of delete it depends on delete_tokens. Not so clear when configuring the app, the first thing that comes to mind is that it should be 401 (any token won't match configured ones, they are unset), but in fact 404 seems safer in this case.

So I think we can keep it as is, probably improve the documentation related to the config - if delete_tokens unset then delete endpoint won't be exposed at all.

lack of auth_tokens - allows the access (btw, is it intended? because sound like a bug)

Yeah, this is intended. Do you think we should have an empty list of tokens for allowing access? It is not a big deal imo.

I thought more about explicit configuration flag, like disable_auth. Just looks less fragile when configuring the app, but the behavior itself is ok, just there was no any comments and I have been concerned. I've added comment & logging with this behavior anyway.

I saw a derive macro on the endpoints that we want to protect but can you tell me how that works?

Actually it works pretty straightforward:

Middleware which extracts some "authority" entity from request (in our case we do extract TokenType)
it adds specific type (AuthDetails<T>) to extensions

macro actually helps to get rid of the following boilerplate, it expands to something like that:

async fn endpoint(details: AuthDetails) -> HttpResponse {
if details.has_authority(TokenType::Auth) {
    // body of our function
} else {
   // error we want to see, defaults to `Forbidden`
}
}

But we can write it manually as well

Also it would be nice to get some information in terms of which version of Actix do you support, what are the plans for the future (will there be any breaking changes) etc.

actix-web was the first framework within this ecosystem, there is mention here: https://github.com/DDtKey/protect-endpoints/tree/main/actix-web-grants#supported-actix-web-versions, any major update of the actix-web gonna be supported with new major version of actix-web-grants

Recently I've delivered a huge update and I'd say it's likely long-term vision of API. As part of that update I also switched to release-plz (with cargo-semver-checks & git-cliff under the hood), so we have some validations.

Current macro-syntax flexible enough and should fit a lot of cases, including ABAC & RBAC, for example, you can combine the conditions in Rust-friendly way, using all & any:

// snippet from examples
#[actix_web_grants::protect(any("ADMIN", expr = "user.is_super_user()"), ty = "Role")]
async fn admin_or_super_user(user_id: web::Path<i32>, user: web::Data<User>) -> HttpResponse {
    HttpResponse::Ok().body("some secured response")
}

So I don't expect the breaking changes in the near future, and if they would be - it will be major (semver) update without breaking existing applications, and with back-porting the changes if they're compatible & makes sense.

DDtKey commented 9 months ago

Great! Just let me know if you have problems / need guidance while adding them.

I've added fixtures, it looks pretty clear to me, thanks!

Some not related observations:

Would be nice to call teardown even in case of failure, to clean up
macos: sleep doesn't work with time-units (e.g sleep 1s)
macos: there is no fallocate (alternative would be to use dd if=/dev/random of=normalfile count=10000 bs=1024 status=none)

I think would be nice to ensure it works on any platform, or provide fast way to run it with docker for example. See #200

orhun commented 9 months ago

So I think we can keep it as is, probably improve the documentation related to the config - if delete_tokens unset then delete endpoint won't be exposed at all.

Makes sense - would you be interested in making a PR to update README.md about this?

I thought more about explicit configuration flag, like disable_auth [...] I've added comment & logging with this behavior anyway.

👍🏼

Actually it works pretty straightforward:

Thanks for the explanation! It is more clear now.

I also switched to release-plz (with cargo-semver-checks & git-cliff under the hood)

Aye that's me! Let me know if you need help with any configuration.

Current macro-syntax flexible enough and should fit a lot of cases, including ABAC & RBAC, for example, you can combine the conditions in Rust-friendly way, using all & any:

Wow that's nice!

So I don't expect the breaking changes in the near future, and if they would be - it will be major (semver) update without breaking existing applications, and with back-porting the changes if they're compatible & makes sense.

Alright, sounds good.

DDtKey commented 9 months ago

Makes sense - would you be interested in making a PR to update README.md about this?

Take a look at #202, please

Aye that's me! Let me know if you need help with any configuration.

Yes, thanks for the wonderful tool! :pray: Actually I've been using it for about a year on different projects! 😅

orhun commented 9 months ago

LGTM! Just one last thing.

I usually put the user-facing changes in the changelog so I guess the only things that have changed are:

we now return 404 instead of 403 for non-exposed endpoints
we don't allow blank (whitespace) tokens

Do we need to mention something else in the changelog?

DDtKey commented 9 months ago

In that case I think these two are enough, I'll update the changelog entry

orhun / rustypaste