core icon indicating copy to clipboard operation
core copied to clipboard

Authenticators factory

Open pieroit opened this issue 1 year ago • 10 comments

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 avatar Jan 26 '24 22:01 pieroit

I guess this is the python file requiring some modification in order to check the auth_key before allowing the websocket connection: https://github.com/cheshire-cat-ai/core/blob/7a99ba2bcb33a95b241ab6944e6288f3a2a4950e/core/cat/routes/websocket.py#L43

diegogosmar avatar Jan 27 '24 16:01 diegogosmar

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

Thanks

pieroit avatar Jan 27 '24 17:01 pieroit

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?

lucapirrone avatar Mar 11 '24 09:03 lucapirrone

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

diegogosmar avatar Mar 11 '24 09:03 diegogosmar

@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 avatar Mar 11 '24 17:03 pieroit

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 avatar Apr 22 '24 18:04 pieroit

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 avatar Apr 22 '24 18:04 pieroit

I'm here!

giovannialbero1992 avatar Apr 22 '24 18:04 giovannialbero1992

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 avatar Apr 24 '24 13:04 pieroit

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

pieroit avatar May 07 '24 11:05 pieroit

@pieroit can we close this issue?

valentimarco avatar Aug 07 '24 17:08 valentimarco