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

(feat): Enabled mTLS in ExtractorAgent, Fixes #989 #1010

Merged
merged 14 commits into from
Nov 12, 2024

Conversation

Default2882
Copy link
Contributor

Context

This change enables the agent to use mTLS by picking up the certificate and private key from config_path. This PR addresses issue #989

What

Made minor changes in ExtractorAgent class in agent.py to pickup the mTLS config from config_path and pass it to IndexifyClient.

Testing

Added 2 UTs -

  1. Initialisation of ExtractorAgent with mTLS config.
  2. Default behaviour.

Contribution Checklist

  • [ DONE ] If the python-sdk was changed, please run make fmt in python-sdk/.
  • [ NOT APPLICABLE ] If the server was changed, please run make fmt in server/.
  • [ DONE ] Make sure all PR Checks are passing.

@diptanu
Copy link
Collaborator

diptanu commented Nov 8, 2024

@Default2882 This requires some more work. Here are some more places where we call the client, we will have to use mTLS for those connections too -

async def download_graph(self, namespace: str, name: str, version: int) -> str:

async def download_input(self, task: Task) -> IndexifyData:


http_client = IndexifyClient(

async with aconnect_sse(

@Default2882
Copy link
Contributor Author

@diptanu Added the changes suggested by you. Please take a look and let me know.

Copy link
Collaborator

@diptanu diptanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Default2882 This looks wonderful, I have one main comment for you. Do you want to test this end to end by configuring NGINX or some other reverse proxy infront of the server which does mTLS and try connecting the Executor and see if you can run the tests end to end ?

If you haven't done this before, here are some tutorials - https://medium.com/@mahernaija/how-to-configure-mutual-tls-mtls-for-nginx-736dec9f819d
https://dev.to/darshitpp/how-to-implement-two-way-ssl-with-nginx-2g39
https://gist.github.com/jeduardo/8a4c4465e87767c42ffcdc6b3e9e8396

python-sdk/indexify/common_util.py Show resolved Hide resolved
@Default2882
Copy link
Contributor Author

Default2882 commented Nov 11, 2024

@Default2882 This looks wonderful, I have one main comment for you. Do you want to test this end to end by configuring NGINX or some other reverse proxy infront of the server which does mTLS and try connecting the Executor and see if you can run the tests end to end ?

If you haven't done this before, here are some tutorials - https://medium.com/@mahernaija/how-to-configure-mutual-tls-mtls-for-nginx-736dec9f819d https://dev.to/darshitpp/how-to-implement-two-way-ssl-with-nginx-2g39 https://gist.github.com/jeduardo/8a4c4465e87767c42ffcdc6b3e9e8396

Here's the step which I used for testing -

  1. install nginx using brew install nginx
  2. check if nginx was correctly installed brew services start nginx and go to localhost:8080
  3. stop the server with brew services stop nginx
  4. go to /opt/homebrew/etc/nginx/
  5. add proxy_pass http://127.0.0.1:8900/; in location block of nginx.conf file to setup reverse proxy.
  6. start the indexify server using cargo run, and then start nginx with brew services start nginx, now
    indexify will be available from localhost:8080 as well as localhost:8900.
  7. generate the certificates for client and server using this script -
#!/bin/bash

mkdir nginx-certs
cd nginx-certs
# Using the -nodes flag here so it does not ask for any password when exporting the key
openssl req -subj '/CN=ssl.test.local' -x509 -new -newkey rsa:4096 -keyout key.pem -out cert.pem -sha256 -days 365 -nodes -addext "keyUsage = digitalSignature,keyAgreement" -addext "extendedKeyUsage = serverAuth, clientAuth" -addext "subjectAltName = DNS:ssl.test.local, DNS:localhost, IP:127.0.0.1"
# The PCKS12 export will ask for a password. I will use 'test' again and will refer it in the final curl test command
openssl pkcs12 -export -out client.p12 -inkey key.pem -in cert.pem
openssl verify -CAfile cert.pem cert.pem
  1. Add thw following config in your nginx.conf -
    server {
        listen       4444 ssl;
        server_name  localhost;

	ssl_certificate path/to/cert.pem;
	ssl_certificate_key path/to/key.pem;
	ssl_client_certificate path/to/cert.pem;
	ssl_verify_client optional;
	ssl_session_timeout 5m;
	ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
	ssl_prefer_server_ciphers on;
	ssl_ciphers "EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH";
	  
	add_header Strict-Transport-Security "max-age=31536000";

        location / {
            root   html;
            index  index.html index.htm;
	    proxy_pass  http://127.0.0.1:8900/;
        }
    }
 
  1. Modify the config.yaml in the python-sdk
  2. add INDEXIFY_URL=localhost:4444 environment variable
  3. Run the executor with poetry run indexify-cli executor --config-path config.yaml --server-addr localhost:4444
  4. run the integration tests.

Couple of questions -

  1. The registration of the executor fails with the log registration Error: failed to register: and nothing else at line - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L436 How do I run executor in debug mode?
  2. Here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L368 "http" is hardcoded, I am pretty sure I am supposed to remove that.
  3. Do we have to support websockets here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L102 ? I did not look into it, but i think we have to setup nginx for websockets differently.
  4. Similar question as 3 but for websocket without encryption here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L106
  5. is line 96 till 101 needed - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L96 ? We are not using _ssl_contex anywhere.

@diptanu
Copy link
Collaborator

diptanu commented Nov 11, 2024

  1. The registration of the executor fails with the log registration Error: failed to register: and nothing else at line - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L436 How do I run executor in debug mode?

Add whatever debug code you want, and pip install . or poetry install to install the library locally and see your debug logs.

  1. Here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L368 "http" is hardcoded, I am pretty sure I am supposed to remove that.

Yes, you are supposed to remove that and use https if we are setting certificates and such in the config.

  1. Do we have to support websockets here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L102 ? I did not look into it, but i think we have to setup nginx for websockets differently.

We used to have web sockets back in the day, not anymore. You can remove web sockets related config from the executor.

  1. Similar question as 3 but for websocket without encryption here - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L106

See above

  1. is line 96 till 101 needed - https://github.com/tensorlakeai/indexify/blob/main/python-sdk/indexify/executor/agent.py#L96 ? We are not using _ssl_contex anywhere.

You can remove ssl context too, we were using it for websockets.

@diptanu diptanu merged commit 9593b38 into tensorlakeai:main Nov 12, 2024
5 checks passed
@Default2882 Default2882 deleted the mTLS branch November 12, 2024 08:19
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