Skip to content
This repository was archived by the owner on Jun 23, 2023. It is now read-only.

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented May 21, 2021

and also

  • fix: session manager gets password from general config
  • fix: session manager salt optionally from global conf
  • chore: session Database init small generalization

@peppelinux peppelinux requested review from nsklikas and rohe May 21, 2021 12:03
@rohe
Copy link
Collaborator

rohe commented May 21, 2021

Realised this should be done a couple of days ago. Glad you did it for me :-)

@rohe
Copy link
Collaborator

rohe commented May 21, 2021

But you should fix the tests.

@peppelinux
Copy link
Member Author

@rohe I fixed the tests ... rather I fixed the pointers in the session database.
I also tested static password and salt in globalconf, it works good as if they were randomic.
Something more, something better than before, thank to bugs!

@rohe
Copy link
Collaborator

rohe commented May 22, 2021

We (I) probably should document the three methods dump/flush/load better.
Here is a start:

The bases for these three methods are the parameter definitions.
Those are the attributes that are affected by the three methods.

dump returns the value in a format that is possible to json dump
flush assigns as value to the attribute the value that is defined in the parameter specification.
load assigns a value to the attribute.

This means that after a flush you MUST do a load to get a working setup.

Having something like salt being randomly defined means that all entities that needs to have the same salt must be loaded with the same state. This means that having anything that should be known to more than one worker being assigned randomly is probably a bad idea. You can make it work by initialising one worker, dump its initial state and then have all the subsequent initialised workers being loaded with the first ones state.

@peppelinux
Copy link
Member Author

peppelinux commented May 22, 2021

We (I) should improve this commit making load_key and load_salt aware of the random value, stored elsewhere in two private attributes of sman objects, this Will prevent a random value Will be generated After any flush.

But multiple workers Will Always have their own randomic, that's why I put the static password and salt parameters in the global configuration.

These will prevent any randomness.

So, what to do now with this PR?

@peppelinux
Copy link
Member Author

salt value in parameters dict Is "".
This caused that salt being "" After a flush without any load (It May happen in some condition) and self.salt, the originari one, were overwritten by this "" runtime

The static password/Key configurations fixed this behaviour by-default and an additional commit can make a immutabile random salt and Key value instead of calling eberytime the rnd() finction

@rohe
Copy link
Collaborator

rohe commented May 22, 2021

You should NEVER do a flush without a load. That an invitation to disaster !
flush resets all defined parameters to their default value (defined in parameter).
Default values all are empty values: "", [], {}, 0, False, ..

@rohe
Copy link
Collaborator

rohe commented May 22, 2021

If you don't want random values per worker, then the value MUST be defined in the configuration (basically being static) or in a file referenced by the configuration (a la session_key).

@peppelinux
Copy link
Member Author

You should NEVER do a flush without a load. That an invitation to disaster !
flush resets all defined parameters to their default value (defined in parameter).
Default values all are empty values: "", [], {}, 0, False, ..

It could happen due to unhandled exceptions.
With this PR I can make many flush without breaking anything.

This PR also enable static password and salt.

I'llonly do a another commit to improve the init_db method as we shared in the previous posts.

Can I intend that we agree and then go ahead?

@peppelinux
Copy link
Member Author

Once this PR Will be merged then se could see this
#60

session_key, sub_funcs, password and salts MAY be under a session manager umbrella, as a opconfiguration root mode and not sparse as they are. Hope to meet your agreement, this Will make Easier to read/understand the configuration and obviously make a Better documentation 💪

@rohe
Copy link
Collaborator

rohe commented May 22, 2021

I think we must be clear about what is under the dump/flush/load umbrella and what isn't.
If we allow special handling of some attributes that implements something in-between persistent/non-persistent storage we are not making it any easier either for us or for users of the package.

@peppelinux
Copy link
Member Author

They all are related to session manager, Just this. All this parameters belongs to session manager.

But this aspect Is out of scope in this PR, i talked too much on things not related to the scope of this PR. It Just fix few small bugs and now my implementation of storage, in django-oidc-op develop branch, works great.

This afternoon I'll add another commit to follow what shared here between US but feel free to suggest, tell, do what you intende to be Better for the roadmap. I accept task 😊

@peppelinux
Copy link
Member Author

peppelinux commented May 22, 2021

Ok, made it, unit tests works great and also added another test to deal with the issues it fixes and the feature I want to push with this contribution:

@rohe my feature make a db consistent after a flush and a way to have always the original key/salt in case of disaster.
I just added more resilience and finally, we have what you say and at the same time what I want to have:

  • fluhs/load more fault tolerant (in case of unhandled exception that would let the db inconsistent)
  • password/salt taken, optionally, from the global configuration
  • flush doesn't destroy self.key and self.salt anymore and sman always can re init its db, getting the original values
  • the original values are now protected (readonly setattr _key and _salt) in a way that they can't be overritten runtime.

a wonderful day, anyway

image

@peppelinux
Copy link
Member Author

Last commit improves Documentation and definitively closes #50

tests/test_23_oidc_registration_endpoint.py::TestEndpoint::test_register_alg_keys
  /home/wert/DEV3/OIDC-Project/env/lib/python3.7/site-packages/cryptojwt/utils.py:257: DeprecationWarning: owner is deprecated; use issuer_id
@peppelinux
Copy link
Member Author

I think that conf dict should be readonly.
@rohe do we have to do something in Server configuration and OPconfiguration class?

Once it's created a lock state should prevent any other changes in the future

@rohe
Copy link
Collaborator

rohe commented May 23, 2021

Agree that it should be read only.
Realised that we didn't have support for token endpoint in an OAuth2 Authorization Server so working on it right now. Meant among other things that I had to create ASConfiguration.

Not sure how you would enforce a lock. If someone wanted to get around it, it would be hard to stop.
But it had to demand some work, couldn't be done by default which I guess is as far as we can get.

@rohe
Copy link
Collaborator

rohe commented May 24, 2021

Included in #65

@rohe rohe closed this May 24, 2021
@peppelinux peppelinux deleted the sman_flush branch May 26, 2021 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants