Nginx storage authentication #38

bartoszbetka · 2018-01-09T14:47:51Z

Resolves #19

cameel

I have fixed the small issues and pushed a few fixups. Please review them.

Please fix the remaining issues. Mostly in nginx-storage/default.conf.j2.

cameel · 2018-01-11T08:38:00Z

kubernetes/create-config-maps.sh

@@ -14,3 +14,7 @@ kubectl create configmap nginx-config                               \
 kubectl create configmap concent-api-settings                                \
    --from-file=local_settings.py=config-maps/concent-api/local_settings.py  \
    --from-literal=__init__.py=
+
+kubectl create configmap gatekeeper-api-settings                                \
+    --from-file=local_settings.py=config-maps/gatekeeper/local_settings.py      \


We don't have a container called gatekeeper-api. This should be called gatekeeper-settings, not gatekeeper-api-settings.

cameel · 2018-01-11T08:39:14Z

kubernetes/services/gatekeeper.yml.j2

+        run: gatekeeper
+    spec:
+      containers:
+        - name:             concent-api


The container is running the gatekeeper not concent API so it does not make sense to call it concent-api. This is misleading. It should be called gatekeeper.

cameel · 2018-01-11T08:40:24Z

kubernetes/create-services.sh

@@ -4,4 +4,5 @@

 kubectl create --record --filename services/concent-api.yml
 kubectl create --record --filename services/nginx-storage.yml
+kubectl create --record --filename services/gatekeeper.yml
 kubectl create --record --filename services/nginx-proxy.yml


Services should be created and destroyed in the right order. nginx instances should be created last and destroyed first because they open access to the cluster.

cameel · 2018-01-11T08:41:52Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+          proxy_method      POST;
+          proxy_pass        $gatekeeper_api_backend/gatekeeper/upload-auth/$http_concent_upload_path;
+          proxy_set_header  Concent-Upload-Path "";
+          proxy_set_header  Content-Length "";


Why are you removing Content-Length? Gatekeeper will need to be able to access it in later releases.

cameel · 2018-01-11T08:44:06Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+
+    location @error_401 {
+        default_type application/json;
+        return 401 '{"error_message": "Access denied.", "error_code": "Unauthorized"}';


Your error responses are inconsistent. Here you have error_message and error_code but a few lines above it's message and code. We need to stick to a single format of error responses to make it easy for the client to parse them.

cameel · 2018-01-11T08:50:24Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+        internal;
+        proxy_pass                $gatekeeper_api_backend/gatekeeper/download-auth/$download_url;
+        proxy_pass_request_body   off;
+        proxy_set_header          Content-Length "";


Why are you removing the body and Content-Length? Doesn't auth_request strip the body automatically?

According to nginx site: https://www.nginx.com/resources/admin-guide/restricting-access-auth-request/ request body is discarded for authentication subrequests, so we will need to removing the body and Content-Length.

But if it's discarded, it should not be necessary, right? What happens in gatekeeper if you remove these lines?

I not sure what happend, because gatekeeper not work properly now(problem with golem token)
Without this header, I occur additional errors:

download-auth: [DEBUG] Ignoring connection reset

update-auth: ERROR | Request body exceeded settings.DATA_UPLOAD_MAX_MEMORY_SIZE

In update-auth looks like subrequest send file in body to the django.
Smaller file occur next problem:

[DEBUG] Ignoring EPIPE
I think it not work porperly without this header.

OK. Looks like auth_request does not remove the body after all. So we do have to remove it.

cameel · 2018-01-11T08:51:08Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+        proxy_pass                $gatekeeper_api_backend/gatekeeper/download-auth/$download_url;
+        proxy_pass_request_body   off;
+        proxy_set_header          Content-Length "";
+        proxy_set_header          X-Original_URI $request_uri;


Why do you need this header? And shouldn't it be called X-Original-URI?

According to nginx site: https://www.nginx.com/resources/admin-guide/restricting-access-auth-request/ this header pass the full original request URI with arguments but It's look like without it, also work. I find also this topic: bitly/oauth2_proxy#247

I know, but do you use it? If not then it's not necessary here.

Ok. I delete it.

cameel · 2018-01-11T08:51:27Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+          proxy_pass        $gatekeeper_api_backend/gatekeeper/upload-auth/$http_concent_upload_path;
+          proxy_set_header  Concent-Upload-Path "";
+          proxy_set_header  Content-Length "";
+          proxy_set_header  X-Original_URI $request_uri;


Why do you need this header? And shouldn't it be called X-Original-URI?

cameel · 2018-01-11T08:53:10Z

kubernetes/config-maps/nginx-storage/default.conf.j2

-        limit_except GET { deny all; }
+        limit_except        GET { deny all; }
+        auth_request        /download-auth/;
+        auth_request_set    $auth_status $upstream_status;


What is this for? I don't see this variable used anywhere else in this file. Is it needed here?

Ok. So, I forgot to remove these variables, their goal was to check the http status during debugging.

cameel · 2018-01-11T09:08:49Z

kubernetes/config-maps/nginx-storage/default.conf.j2

+        limit_except        GET { deny all; }
+        auth_request        /download-auth/;
+        auth_request_set    $auth_status $upstream_status;
+        if ($request_uri ~* "/download/(.*)") {


Why case-insensitive regex match (~*) and and not a normal one (~)?

What will the match be if request_uri looks like this: /download/a/b/c/download/file.jpg?

According to this post https://serverfault.com/questions/801356/what-is-the-difference-between-nginx-and-regexes, version with ~* is matching as a case-insensitive regular expression.

So if request look like this download/a/b/c/download/file.jpg, it change properly and go to gatekeeper in this form of request /gatekeeper/download-auth/a/b/c/download/file.jpg

According to this post https://serverfault.com/questions/801356/what-is-the-difference-between-nginx-and-regexes, version with ~* is matching as a case-insensitive regular expression.

I'm asking why do you need a case-insensitive match here.

bartoszbetka requested a review from cameel January 9, 2018 14:47

cameel force-pushed the nginx-storage-authentication branch from 7cd21e3 to eacadfb Compare January 10, 2018 14:03

bartoszbetka force-pushed the nginx-storage-authentication branch from eacadfb to 6c22a7e Compare January 10, 2018 15:34

cameel suggested changes Jan 11, 2018

View reviewed changes

Gr33ny and others added 3 commits January 12, 2018 16:15

Add gatekeeper to configuration files

4b9e5e6

Add authentication support for upload and download

5e536fd

nginx-storage: Remove the code field from success responses

585ea8e

cameel force-pushed the nginx-storage-authentication branch from 91c54b8 to 585ea8e Compare January 12, 2018 15:16

cameel merged commit 585ea8e into master Jan 12, 2018

cameel deleted the nginx-storage-authentication branch January 12, 2018 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nginx storage authentication #38

Nginx storage authentication #38

bartoszbetka commented Jan 9, 2018 •

edited

Loading

cameel left a comment

cameel Jan 11, 2018

cameel Jan 11, 2018

cameel Jan 11, 2018

cameel Jan 11, 2018

cameel Jan 11, 2018

cameel Jan 11, 2018

bartoszbetka Jan 11, 2018

cameel Jan 11, 2018

bartoszbetka Jan 11, 2018

cameel Jan 12, 2018

cameel Jan 11, 2018

bartoszbetka Jan 11, 2018

cameel Jan 11, 2018 •

edited

Loading

bartoszbetka Jan 11, 2018

cameel Jan 11, 2018

cameel Jan 11, 2018

bartoszbetka Jan 11, 2018

cameel Jan 11, 2018 •

edited

Loading

bartoszbetka Jan 11, 2018

cameel Jan 11, 2018

Nginx storage authentication #38

Nginx storage authentication #38

Conversation

bartoszbetka commented Jan 9, 2018 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszbetka commented Jan 9, 2018 •

edited

Loading

cameel Jan 11, 2018 •

edited

Loading

cameel Jan 11, 2018 •

edited

Loading