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

Authenticators factory #690

Open
pieroit opened this issue Jan 26, 2024 · 10 comments
Open

Authenticators factory #690

pieroit opened this issue Jan 26, 2024 · 10 comments
Assignees
Labels
endpoints Related to http / ws endpoints enhancement New feature or request

Comments

@pieroit
Copy link
Member

pieroit commented Jan 26, 2024

An old design decision was to lock under AUTH_KEY only http endpoints.
Let's make the key cover all the endpoints, included websocket

EDIT: A better idea is to have AUTH_KEY for http endpoints and a different WS_AUTH_KEY for authenticating websocket

P.S. check if client libs need an update

@pieroit pieroit added enhancement New feature or request endpoints Related to http / ws endpoints labels Jan 26, 2024
@diegogosmar
Copy link

I guess this is the python file requiring some modification in order to check the auth_key before allowing the websocket connection:

async def websocket_endpoint(websocket: WebSocket, user_id: str = "user"):

@pieroit
Copy link
Member Author

pieroit commented Jan 27, 2024

Let's ask @EugenioPetulla where is better to force api auth because he handled the http one

Thanks

@lucapirrone
Copy link
Contributor

Hi guys, I'll give my opinion on this.
HTTP APIs and websocket communication play two different roles within the cat. HTTP APIs are used to administer the cat while websocket communication is used to let users communicate.

Using the same key for both the websocket and the http API would mean having to make the user administrator key public, causing a major security problem.

If you wanted to make the websocket secure, you should extend the cat with a plugin in order to implement a custom authorization logic based on your user management system.

For example, in an application with a token-based authentication system, you could insert the user token into the metadata of websocket messages and check the validity of the token in the "Before agent starts" hook.

What do you think about it?

@diegogosmar
Copy link

Hi Luca,
I agree we should keep separate the HTTP RESTful API from the Websocket protocol.
However, I'm not sure we should have an external plugin to grant the Websocket security.

How about to add an APIKey related to the websocket initial authentication (different from the APIKey used for the Rest API)?
Ideally it would be to add the possibility of using an OAuth Token:
OAuth Tokens: For more robust security, especially in systems where users have individual accounts, consider using OAuth tokens. After the user logs in via a secure HTTP connection, your server issues a token that the client then presents when establishing the WebSocket connection.

I've also noticed that in the documentation examples the websockets are set not to use a secure connection (ws instead of wss):
config = ccat.Config(
base_url=cat_url,
port=1865,
user_id="user",
auth_key=auth_key,
secure_connection=False # Indicates an insecure connection (ws://)
)

I didn't try to set it to True and see if the WSS are working, however we should have a try to be sure that we support encrypted websocket communications.

Thanks!
ciao
Diego

@pieroit
Copy link
Member Author

pieroit commented Mar 11, 2024

@lucapirrone @diegogosmar ok let's go with two different env variables, one for http and one for websocket.
Makes sense!

@diegogosmar can you do a check about the wss issues?

@pieroit
Copy link
Member Author

pieroit commented Apr 22, 2024

PROPOSAL 1

vote = mh.execute_hook([], payload, cat)
# [True, True, False]

return np.all(vote)
@hook
def authorize([], payload, cat):
    
    user_id = cat.user_id # payload.user_id
    token = payload.token

    # my logic (API_KEY or IDP)

    cat.working_memory.permissions = ["fuck", "around"]
    vote.append(True)
    return vote

@pieroit
Copy link
Member Author

pieroit commented Apr 22, 2024

PROPOSAL 2

list_authenticators_default = [ApiKeyAuthenticator, OAuthAuthenticator]

authenticators = mh.execute_hook(
    "allowed_authenticators", list_authenticators_default, cat=None
)

# Admin choses authenticator
class MyAuthenticator(CatAuthenticator):
    
    http_key: str
    ws_key : str
    
    def auth_http(self, payload, cat)
        return True

    def auth_websocket(self, payload, cat)
        return True

@hook
def allowed_authenticators(auth_list, cat):
    auth_list.append(MyAuthenticator)
    return auth_list

@pieroit pieroit changed the title Have AUTH_KEY also impact ws endpoints Authenticators factory Apr 22, 2024
@giovannialbero1992
Copy link
Contributor

I'm here!

@pieroit
Copy link
Member Author

pieroit commented Apr 24, 2024

It's mostly a copy paste of the llm / embedder factory

For exmaple on the embedders these are the key files:
Factory:
https://github.com/cheshire-cat-ai/core/blob/develop/core/cat/factory/embedder.py
Load the chosen one:
https://github.com/cheshire-cat-ai/core/blob/develop/core/cat/looking_glass/cheshire_cat.py#L142
Endpoints:
https://github.com/cheshire-cat-ai/core/blob/develop/core/cat/routes/embedder.py
Tests:
https://github.com/cheshire-cat-ai/core/blob/develop/core/tests/routes/embedder/test_embedder_setting.py

Should be easier for Authenticators because we try yo couple LLM/Embedder vendors, while in the case of auth there are no couplings

Let me know if I can help, also on a draft PR

@pieroit
Copy link
Member Author

pieroit commented May 7, 2024

After dev meeting, to improve on #794 :

BaseAuth will do something like (pseudo-code):

class BaseAuth():

  def is_master_key(request):
    # current logic with API_KEY, if present in .env
    return request.header["access_token"] == getenv("API_KEY")

  def is_http_allowed(request):
    return is_master_key(request)

  def is_ws_allowed(websocket):
    return True

In plugins you can register a subclass

class MyAuth(BaseAuth):
  
  # may or may not be overridden,
  # if you do, you lock out the admin and any community client relying on API_KEY
  def is_master_key(request):
      return False

  def is_http_allowed(request):
    # your logic
    return True | False

  def is_ws_allowed(websocket):
    # your logic
    return True | False

Instance of this class is selected via admin/endpoint and it is stored in cat.auth

All endpoints will have a Depends to filter by authenticator

@router.get("/something")
def some_endpoint(request, payload, Depends(http_auth)):
  pass

http_auth will use master_key and custom access in OR

def http_auth(app, request):
    cat = app.state.ccat # not sure if we can reach `stray` directly
    if not cat.auth.is_master_key(request) or cat.auth.is_http_allowed(request):
       raise Exception

Same logic for a Depends(ws_auth)

Admin will be saved by just using the API_KEY as it is now, and in the future we may have a custom temp token for the admin.

Hope I'm seeing everything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
endpoints Related to http / ws endpoints enhancement New feature or request
Projects
Status: 📋 Backlog
Development

No branches or pull requests

4 participants