-
Notifications
You must be signed in to change notification settings - Fork 362
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
Comments
I guess this is the python file requiring some modification in order to check the auth_key before allowing the websocket connection: core/core/cat/routes/websocket.py Line 43 in 7a99ba2
|
Let's ask @EugenioPetulla where is better to force api auth because he handled the http one Thanks |
Hi guys, I'll give my opinion on this. 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? |
Hi Luca, How about to add an APIKey related to the websocket initial authentication (different from the APIKey used for the Rest API)? I've also noticed that in the documentation examples the websockets are set not to use a secure connection (ws instead of wss): 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! |
@lucapirrone @diegogosmar ok let's go with two different env variables, one for http and one for websocket. @diegogosmar can you do a check about the wss issues? |
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 |
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 |
I'm here! |
It's mostly a copy paste of the llm / embedder factory For exmaple on the embedders these are the key files: 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 |
After dev meeting, to improve on #794 :
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 All endpoints will have a @router.get("/something")
def some_endpoint(request, payload, Depends(http_auth)):
pass
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 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 can we close this issue? |
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
The text was updated successfully, but these errors were encountered: