Make symkey an optional kwarg. #324

decentral1se · 2017-04-05T17:53:34Z

Any changes relevant to users are recorded in the CHANGELOG.md.
The documentation has been updated, if necessary.

Follows from #322.

tpazderka

I think that we shouldn't provide a default value to something that is security related. Rather define it as None and raise an ImproperlyConfigured error or similar if user is trying to use it without defining a proper value.

VJalili · 2017-04-05T18:43:13Z

Agreed. I think the function signature is fine, but it's required to check for authentication method. Something link:
if authenticationMethod == symkeyauthn && symkey="" then raise ImproperlyConfigured

@lwm Could you please update your PR accordingly ?

VJalili

Making symkey optional triggers following error for me:

File "/pyoidc/src/oic/utils/http_util.py", line 508, in get_cookie_value\n value, _ts, typ = txt.split("::")\n', ' ValueError: need more than 1 value to unpack\n'

tpazderka · 2017-04-06T07:06:57Z

That probably is a side-effect of what I have pointed out. Empty string is used in creation of the cookie.

decentral1se · 2017-04-06T15:10:23Z

Ah yes, I'll need to work a bit more on this and make some tests. Thanks for review 👍

decentral1se · 2017-04-07T21:33:49Z

Breaking changes! 💣 I'll need to bump the version, I suppose. Getting close?

tpazderka · 2017-04-09T21:48:13Z

src/oic/oic/claims_provider.py

@@ -69,7 +69,7 @@ class ClaimsServer(Provider):
    def __init__(self, name, sdb, cdb, userinfo, client_authn, urlmap=None,
                 ca_certs="", keyjar=None, hostname="", dist_claims_mode=None):
        Provider.__init__(self, name, sdb, cdb, None, userinfo, None,
-                          client_authn, "", urlmap, ca_certs, keyjar,
+                          client_authn, rndstr(), urlmap, ca_certs, keyjar,


This can probably cause issues...

Yes, you're right, will set to None instead.

tpazderka · 2017-04-09T21:49:54Z

src/oic/oauth2/provider.py

                 ca_bundle=None, verify_ssl=True, default_acr="",
                 baseurl='', server_cls=Server, client_cert=None):
+
+        if symkey is not None and symkey == "":


Maybe we can postpone this check till the point where the symkey is actually needed? Otherwise this check seems a little bit pointles since it passes for None which will break stuff.

I see what you mean, I'll try to find the point where it is used.

tpazderka · 2017-04-09T21:51:04Z

Sure, we can remmember to bump the version, but we should wait so we do not release that often.

tpazderka · 2017-04-09T21:51:58Z

Also you have a lot of unrelated cleanup changes. I would rather see them in a separate issue targeted at cleanup.

decentral1se · 2017-04-13T14:08:27Z

Also you have a lot of unrelated cleanup changes. I would rather see them in a separate issue targeted at cleanup.

Yep, fair point, please see #329.

decentral1se · 2017-04-13T14:30:10Z

Validate symkey is not the empty string in CookieDealer and SymkeyAuthn.
Write tests to cover these changes
~~Add some documentation (better start now than never)~~ (no time again!)

I want to remove the level of indentation for this function and find this to be the simplest way to do that. I think it improve readability (I will add to this functions implementation in future commits).

This new class is useful to show some user error when configuring the Provider. I imagine, as we start to identify more which arguments/values are security related, we will start to use this more.

Please see #322.

In this commit, I track the usage of the `symkey` to the root in `CookieDealer` and `SymKeyAuthn` and assert that it is never the empty string. Please see the following comment for background on this change: #324 (review)

I have to add a __init__.py so that pytest can figure out where the conftest is. Closes #361.

decentral1se · 2017-06-04T23:01:27Z

OK gentlemen, please review this again! I've given it another go 👍

coveralls · 2017-06-04T23:04:52Z

Coverage increased (+0.05%) to 62.93% when pulling 3e7e291 on lwm:optional-symkey into b7a44a9 on OpenIDC:master.

schlenk

Please fix at least the return case.

schlenk · 2017-06-05T09:48:06Z

src/oic/utils/authn/user.py

@@ -387,6 +388,10 @@ class SymKeyAuthn(UserAuthnMethod):

    def __init__(self, srv, ttl, symkey):
        UserAuthnMethod.__init__(self, srv, ttl)
+
+        if symkey is not None and symkey == "":


The actual check is even stricter, due to the minimal aes keylength, that is enforced in oic.util.aes.build_cipher.

Maybe the aes code could provide a method to check for the minimal keylength, so we could raise ImproperlyConfigured for that too, instead of the current AssertionError/FailedAuthentication which is misleading?

The actual check is even stricter, due to the minimal aes keylength, that is enforced in oic.util.aes.build_cipher. ... Maybe the aes code could provide a method to check

I don't see the connection from the Provider.symkey to the oic.util.aes.build_cipher? Does one of the provider classes call this function? We put the check here due to the idea in this comment.

You are right (in a way).

The only usecase for using the Provider.symkey currently is to use it as a key for AES encryption. And AES encryption needs a minimum key length (as checked in build_cipher). So it makes no sense to have a symkey that is too short to use as an AES key. (e.g. just 2 Bytes or something like that).

Currently, you can set a symkey like 'ab' and all the setup/init works, but you blow up at runtime, when you get FailedAuthentication or other AssertionErrors from aes.encrypt(). So it would be user friendlier to prevent a too short to be useful symkey too, instead of just empty and None.

Aha! I see, I see. Thanks for the explanation. I'll follow up with a key length check in #367.

schlenk · 2017-06-05T09:52:58Z

src/oic/utils/http_util.py

-                if not getattr(srv, param, None):
-                    setattr(srv, param, rndstr().encode("utf-8"))
+        if not srv:
+            return


Why not raise an exception here? Every other method in the class will blow up with an Exception if self.srv is None, so the class is in a broken state without it.

Because the original code did not either and I wasn't sure if this was acceptable. I'll patch that up. Please, @rohe, can you confirm this?

I'd actually be happier with making this a separate ticket if you don't mind @schlenk. I have preserved the original behaviour (I think!) in this change but your recommendation (which I think is totally valid!) will bring something new. I've created #365 for that.

ok, fine with me.

rohe · 2017-06-07T06:31:17Z

Over the time I've been working on this library I've gone back and forth between throwing an exception or returning something to signal that an error occurred.
Lately I'm leaning more toward the 'throw an exception' camp so I would be OK with changing the code to raise an exception.

VJalili approved these changes Apr 5, 2017

View reviewed changes

tpazderka requested changes Apr 5, 2017

View reviewed changes

VJalili suggested changes Apr 5, 2017

View reviewed changes

tpazderka requested changes Apr 9, 2017

View reviewed changes

decentral1se mentioned this pull request Apr 13, 2017

Cosmetics - inlining and comment removal. #329

Merged

decentral1se changed the title ~~Make symkey an optional kwarg.~~ WIP: Make symkey an optional kwarg. Apr 13, 2017

decentral1se added 5 commits June 4, 2017 22:35

Return early if srv is None.

0b3e118

I want to remove the level of indentation for this function and find this to be the simplest way to do that. I think it improve readability (I will add to this functions implementation in future commits).

Start release notes for 0.11.0.0 and #324 changes.

f0b7218

Remove white space.

fe3cbbf

Add new exception class: ImproperlyConfigured.

aee070e

This new class is useful to show some user error when configuring the Provider. I imagine, as we start to identify more which arguments/values are security related, we will start to use this more.

Make the symkey argument optional.

0c1da70

Please see #322.

decentral1se mentioned this pull request Jun 4, 2017

Create a provider pytest fixture for all tests #361

Closed

decentral1se added 5 commits June 4, 2017 23:53

Validate symkey is not the empty string.

533dc8f

In this commit, I track the usage of the `symkey` to the root in `CookieDealer` and `SymKeyAuthn` and assert that it is never the empty string. Please see the following comment for background on this change: #324 (review)

Add conftest and add first fixture - a provider!

a18f52c

I have to add a __init__.py so that pytest can figure out where the conftest is. Closes #361.

Add test for SymKeyAuthn optional symkey.

547fff0

Add test for CookieDealer optional symkey.

55c1e2e

Sort imports.

3e7e291

decentral1se requested review from rohe, schlenk and tpazderka June 4, 2017 23:02

decentral1se self-assigned this Jun 4, 2017

decentral1se added the enhancement label Jun 4, 2017

decentral1se added this to the P2: SHOULD milestone Jun 4, 2017

decentral1se changed the title ~~WIP: Make symkey an optional kwarg.~~ Make symkey an optional kwarg. Jun 4, 2017

schlenk requested changes Jun 5, 2017

View reviewed changes

decentral1se mentioned this pull request Jun 5, 2017

Ensure that CookieDealer.srv is configured properly #365

Closed

schlenk approved these changes Jun 5, 2017

View reviewed changes

decentral1se mentioned this pull request Jun 5, 2017

Also raise ImproperlyConfigured for Provider.symkey bad key length #367

Closed

decentral1se merged commit b0c8982 into CZ-NIC:master Jun 5, 2017

decentral1se deleted the optional-symkey branch June 5, 2017 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make symkey an optional kwarg. #324

Make symkey an optional kwarg. #324

decentral1se commented Apr 5, 2017 •

edited

Loading

tpazderka left a comment •

edited

Loading

VJalili commented Apr 5, 2017 •

edited

Loading

VJalili left a comment

tpazderka commented Apr 6, 2017

decentral1se commented Apr 6, 2017

decentral1se commented Apr 7, 2017 •

edited

Loading

tpazderka Apr 9, 2017

decentral1se Apr 13, 2017

tpazderka Apr 9, 2017

decentral1se Apr 13, 2017

tpazderka commented Apr 9, 2017

tpazderka commented Apr 9, 2017

decentral1se commented Apr 13, 2017

decentral1se commented Apr 13, 2017 •

edited

Loading

decentral1se commented Jun 4, 2017

coveralls commented Jun 4, 2017 •

edited

Loading

schlenk left a comment

schlenk Jun 5, 2017

decentral1se Jun 5, 2017

schlenk Jun 5, 2017

decentral1se Jun 5, 2017 •

edited

Loading

schlenk Jun 5, 2017

decentral1se Jun 5, 2017

decentral1se Jun 5, 2017

schlenk Jun 5, 2017

rohe commented Jun 7, 2017

Make symkey an optional kwarg. #324

Make symkey an optional kwarg. #324

Conversation

decentral1se commented Apr 5, 2017 • edited Loading

tpazderka left a comment • edited Loading

Choose a reason for hiding this comment

VJalili commented Apr 5, 2017 • edited Loading

VJalili left a comment

Choose a reason for hiding this comment

tpazderka commented Apr 6, 2017

decentral1se commented Apr 6, 2017

decentral1se commented Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpazderka commented Apr 9, 2017

tpazderka commented Apr 9, 2017

decentral1se commented Apr 13, 2017

decentral1se commented Apr 13, 2017 • edited Loading

decentral1se commented Jun 4, 2017

coveralls commented Jun 4, 2017 • edited Loading

schlenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentral1se Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohe commented Jun 7, 2017

decentral1se commented Apr 5, 2017 •

edited

Loading

tpazderka left a comment •

edited

Loading

VJalili commented Apr 5, 2017 •

edited

Loading

decentral1se commented Apr 7, 2017 •

edited

Loading

decentral1se commented Apr 13, 2017 •

edited

Loading

coveralls commented Jun 4, 2017 •

edited

Loading

decentral1se Jun 5, 2017 •

edited

Loading