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

Simplify auth mechanism #216

Open
wants to merge 16 commits into
base: stretch-unstable
from

Conversation

@alexAubin
Copy link
Member

commented Aug 20, 2019

Soooo I was crazy enough to try to investigate how to remove the dependency to gnupg which led me to try to understand the spaghetti madness of how/where the authentication happens exactly...

I ended up cleaning and refactorizing various parts of the code to make it less crazy ...

So right now what I got is this (trying to summarizing my understading .. that's still quite complicated but much less than before this PR I guess) :

  • each interface api and cli implement a auth_required method which dynamically check if the request requires to be authenticated. For example, in the context of yunohost, the special route POST /postinstall doesn't require to be authenticated
  • auth_required is used by the actionsmap.py, which if needed loads and triggers the authentication check for the corresponding profile (in our case, basically we only have default...)
  • the loading of the authentication modules (well, ldap..) is done according to the details provided in the actionmap
  • then each interface api and cli implement a _do_authenticate method that allows to request some secret or whatever mechanism for the user to effectively authenticate
  • the actual authentication happens by calling authenticator() which calls __call__ in authenticators/__init__.py itself calling ldap.authenticate(password) (but then there's the whole token mechanism stored with gpg etc...)
  • finally, ldap.is_authenticated is called to check wether the user is or not authenticated now

@alexAubin alexAubin changed the title [wip] Simplify auth mechanism Simplify auth mechanism Aug 23, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Sooo it's mostly done I think though I might still tweak a few minor things or add other tests ... so reviews are welcome if you have the courage for this. Reading commit by commit is encouraged.

@decentral1se : I'm interested if you have some feedback on how I wrote the tests .. I'm not sure that it's 100% the orthodox way of doing it. Also it's a bit tricky because I have to start the api thread in the setup - but then the coverage can't easily be computed. Maybe you know a trick ... (And last : dunno if there's a tricky to be able to run pdb when running tox - or running single test files manually)

hash_ = hashlib.sha256(to_hash).hexdigest()

if hash_ != stored_hash:
raise MoulinetteError('invalid_token')

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 23, 2019

Author Member

Note that this is the critical crypto part where we check the user provided id+token against a hashed id+token stored locally during the initial auth.

This comment has been minimized.

Copy link
@randomstuff

randomstuff Aug 30, 2019

Contributor

It's not super clear to understand what is supposed to be the token, the hash, etc. :)

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 30, 2019

Author Member

Eh yup, I added some big comment (dunno if that's the right place for it) to explain what's happening.

@decentral1se
Copy link
Member

left a comment

I can't pretend to get everything happening here but I've tried! Thanks for this work.

Also it's a bit tricky because I have to start the api thread in the setup - but then the coverage can't easily be computed. Maybe you know a trick ... (And last : dunno if there's a tricky to be able to run pdb when running tox - or running single test files manually)

The API thread stuff looks good to me and I can't see anyone doing it any different than using the yield approach. For running pdb with tox you can do -- -s and it will respond to any pdb.set_trace() statements. You can also use -- -s --pdb to debug on failure. RE: a single test, I typically use -- -k nameoftest and it works for me TM.

self.is_authenticated = True

# Store session for later using the provided (new) token if any
if token:

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

We can loop forever without being able to store the session token and keep being asked to authenticate? Should we also raise an exception here to stop such a loop? Unsure if this can happen but trying to follow along all the same 👨‍💻

"""Retrieve a session and return its associated password"""
try:
# FIXME : shouldn't we also add a check that this session file

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

Perhaps wait for a bug report 😸

tmp_cache = str(tmp_path_factory.mktemp("cache"))
tmp_data = str(tmp_path_factory.mktemp("data"))
tmp_lib = str(tmp_path_factory.mktemp("lib"))
os.environ['MOULINETTE_CACHE_DIR'] = tmp_cache

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

I would recommend to use the monkeypatch fixture for patching the environment. Please see https://docs.pytest.org/en/3.0.1/monkeypatch.html. It means that pytest will take care to clean up any changes to the environment for you. Can stop crazy hard to find env bugs later on for someone else.

authenticate: all
authenticator: default

# only-api:

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

Still needed?

args=([namespace],),
kwargs={"host": "localhost", "port": 12342, "use_websocket": False})
api_thread.start()
time.sleep(0.5)

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

sleep in tests is fine at first but can build up (as others see it is "already being used"). Perhaps a while loop with a counter to break out? Things start faster on some machines, slower on others? Alternatively just leave it (so much to be done now anyway) and wait to hear if it causes issues.

@@ -15,6 +15,9 @@ deps =
pytest-env >= 0.6.2, < 1.0
requests >= 2.22.0, < 3.0
requests-mock >= 1.6.0, < 2.0
toml >= 0.10

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 23, 2019

Member

Please add >= and < bounds as it can help to make the test suite more stable going forward.

@alexAubin alexAubin referenced this pull request Aug 23, 2019
0 of 4 tasks complete
@@ -0,0 +1,10 @@
def testauth_none():

This comment has been minimized.

Copy link
@randomstuff

randomstuff Aug 30, 2019

Contributor

What is this file doing?

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 30, 2019

Author Member

It's meant to implement dummy functions corresponding to the actionsmap in test/actionsmap/moulitest.yml used for "unit" / functional tests


# We store a hash of the session_id and the session_token (the token is assumed to be secret)
to_hash = "{id}:{token}".format(id=session_id, token=session_token)
hash_ = hashlib.sha256(to_hash).hexdigest()

This comment has been minimized.

Copy link
@randomstuff

randomstuff Aug 30, 2019

Contributor

Note: could-possibly use the binary SHA-256 instead of converting to hexadecimal.

to_hash = "{id}:{token}".format(id=session_id, token=session_token)
hash_ = hashlib.sha256(to_hash).hexdigest()

if hash_ != stored_hash:

This comment has been minimized.

Copy link
@randomstuff

randomstuff Aug 30, 2019

Contributor

Would it be useful to use mac.compare_digest(hash_, stored_hash)?

@@ -31,6 +32,7 @@ class BaseAuthenticator(object):

def __init__(self, name):
self._name = name
self.is_authenticated = False

This comment has been minimized.

Copy link
@randomstuff

randomstuff Aug 30, 2019

Contributor

What is this supposed to do? It is set to True at some point and never switched back to False. is_authenticated is global (shared for all users)?

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I tried to understand what's happening (after the patch) but it's not super clear. Below are some notes of what I think I understood.

session.id:

  • session ID
  • created/set on the login page (if there is not existing session ID)

session.tokens:

  • signed dictionnary from profile to secret token
  • per-session secret used to sign session.tokens (this secret is stored in process memory)

Secret token:

  • random token
  • Can be used to login without password?
  • How is it used in practice?

Secret token hash:

  • SHA-256 session_id:profile_token
  • Stored in a per-session file (but not per-profile file?).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.