[SQSERVICES-1913] Move OAuth authentication to nginz by battermann · Pull Request #3086 · wireapp/wire-server

battermann · 2023-02-15T11:06:40Z

No description provided.

beltram · 2023-02-21T14:41:35Z

libs/libzauth/libzauth-c/Makefile


 build-release:
-	cargo build --release
+	cargo build


revert to release mode

beltram · 2023-02-21T14:47:16Z

libs/libzauth/libzauth-c/src/lib.rs

+pub struct OAuthJwk(String);
+
+#[repr(C)]
+#[derive(Clone, Copy, Debug)]


You probably don't want to derive Copy here. It's basically an "implicit" clone. And since your struct holds pointer, it could silently copy those pointers and somehow result in double free errors

beltram · 2023-02-21T14:51:06Z

libs/libzauth/libzauth-c/src/lib.rs

+    }
+}
+
+impl From<io::Error> for OAuthResultStatus {


You probably could have handled all these manual From implementations with the thiserror crate.
But it could be tricky to handle with the discriminant so leave it alone if it's too complex

#[repr(C)] #[derive(Error, Clone, Copy, Debug)] pub enum OAuthResultStatus { Ok, IoError(#[from] io::Error), }

beltram · 2023-02-21T14:51:23Z

libs/libzauth/libzauth-c/src/lib.rs

+impl From<str::Utf8Error> for OAuthResultStatus {
+    fn from(_: str::Utf8Error) -> OAuthResultStatus {
+        OAuthResultStatus::Utf8Error
+    }
+}


Suggested change

impl From<str::Utf8Error> for OAuthResultStatus {

fn from(_: str::Utf8Error) -> OAuthResultStatus {

OAuthResultStatus::Utf8Error

}

}

impl From<str::Utf8Error> for OAuthResultStatus {

fn from(_: str::Utf8Error) -> Self {

Self::Utf8Error

}

}

beltram · 2023-02-21T14:52:33Z

libs/libzauth/libzauth-c/src/lib.rs

+                status: OAuthResultStatus::NullArg,
+            };
+        }
+        if scope.is_null() {


You don't check 'method' ?

Missed that, thanks!

pcapriotti

Looks generally good, as far as I can tell. I did find a potential problem though. See below.

pcapriotti · 2023-02-22T13:00:05Z

libs/libzauth/libzauth-c/src/lib.rs

+            uid: c_str.into_raw(),
+            status: OAuthResultStatus::Ok,
+        }
+    }) {


Can't you use catch_unwind_with here too?

services/nginz/third_party/nginx-zauth-module/zauth_module.c

pcapriotti · 2023-02-22T13:21:33Z

services/nginz/third_party/nginx-zauth-module/zauth_module.c


+static ngx_int_t zauth_token_var_user (ngx_http_request_t * r, ngx_http_variable_value_t * v, uintptr_t _) {
+        ZauthToken const * t = ngx_http_get_module_ctx(r, zauth_module);
+        if (t != NULL && zauth_is_authorized_and_allowed(r)) {


This doesn't look right. First t is cast to ZauthToken and assumed to be a valid pointer to that struct by zauth_is_authorized_and_allowed. Later, it is fetched again and cast to a string pointer.

You might want to store a tagged union in the context, so that this function can discriminate if it's the zauth or oauth case.

Co-authored-by: Paolo Capriotti <paolo@capriotti.io>

…nz' of github.com:wireapp/wire-server into SQSERVICES-1913-oauth-move-o-auth-authentication-to-nginz

pcapriotti

I looked at the C code only this time. Looks good to me modulo the comments below.

pcapriotti · 2023-02-23T12:35:13Z

services/nginz/third_party/nginx-zauth-module/zauth_module.c

+        if (strncmp((char const *) hdr.data, "Bearer ", 7) == 0) {
+                OAuthResult res = oauth_verify_token(key, &hdr.data[7], hdr.len - 7, scope.data, scope.len, r->method_name.data, r->method_name.len);
+                if (res.status == OAUTH_OK) {
+                        ZauthContext * ctx = alloc_oauth_context(r, res.uid);


For OOM-safety, you can add

if (ctx == NULL) return NGX_ERROR;

or similar.

pcapriotti · 2023-02-23T12:35:46Z

services/nginz/third_party/nginx-zauth-module/zauth_module.c

-                if (finaliser == NULL) {
-                        return NGX_ERROR;
+                ZauthContext * ctx = alloc_zauth_context(r, tkn);
+                ngx_int_t e = setup_zauth_context(r, ctx);


battermann changed the base branch from develop to SQSERVICES-1825-be-oauth-refresh-token-generation February 15, 2023 11:07

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 15, 2023

battermann changed the title ~~Sqservices 1913 oauth move o auth authentication to nginz~~ [SQSERVICES-1913] Move OAuth authentication to nginz Feb 15, 2023

battermann added 17 commits February 16, 2023 10:23

rust stuff

3f9410d

rust compiles

a216a47

rust compiles

93083b8

wip

4e0f572

wip

ba70964

wip

4c292bc

works

47ef9f2

scope access

7997155

oauth integration tests only using nginz

673003f

return a tuple

7e2a584

free oauth key mem

d94918f

naming convenetions

37f4cf3

get rid of rust warnings

7de3a20

formatting

341e642

error propagation

80f8409

oauth nginz pass locally

0b5bcec

nginx config for CI

befeca0

battermann force-pushed the SQSERVICES-1913-oauth-move-o-auth-authentication-to-nginz branch from eac9bf9 to befeca0 Compare February 21, 2023 14:34

beltram approved these changes Feb 21, 2023

View reviewed changes

battermann added 7 commits February 21, 2023 15:06

oauth public key for nginz CI

223b0fb

better rust

8ba3108

remove pub keys from brig and galley, fix stuff

45418cd

clean up

4961fef

commt

ec955fb

set oauth_scope in template correctly

367e905

script

4bc1976

pcapriotti reviewed Feb 22, 2023

View reviewed changes

battermann and others added 8 commits February 22, 2023 14:24

Update services/nginz/third_party/nginx-zauth-module/zauth_module.c

90cd5f1

Co-authored-by: Paolo Capriotti <paolo@capriotti.io>

WIP

e26a03e

Merge branch 'SQSERVICES-1913-oauth-move-o-auth-authentication-to-ngi…

1b3c2ca

…nz' of github.com:wireapp/wire-server into SQSERVICES-1913-oauth-move-o-auth-authentication-to-nginz

Merge branch 'SQSERVICES-1913-oauth-move-o-auth-authentication-to-ngi…

bcf6715

…nz' of github.com:wireapp/wire-server into SQSERVICES-1913-oauth-move-o-auth-authentication-to-nginz

clean up

fc92f75

zauth module clean up

296b30a

clean up

924dba4

remove spaces

49d1fd9

battermann marked this pull request as ready for review February 23, 2023 11:47

pcapriotti approved these changes Feb 23, 2023

View reviewed changes

battermann added 2 commits February 23, 2023 13:38

typo in nginx.conf

626ddb1

check for oom error

5b07b11

battermann merged commit 2af8cf7 into SQSERVICES-1825-be-oauth-refresh-token-generation Feb 24, 2023

battermann deleted the SQSERVICES-1913-oauth-move-o-auth-authentication-to-nginz branch February 24, 2023 09:29

battermann mentioned this pull request Feb 24, 2023

OAuth #2989

Merged

2 tasks

Comments

Conversation

battermann commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

battermann commented Feb 15, 2023 •

edited

Loading