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

RADIUS Token Challenge/Response changes #1389

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@droobah
Copy link

droobah commented Jan 18, 2019

In an attempt to migrate from RSA SecurID to privacyIDEA, changes needed to be made to handle the tracking of the state/transaction ID sent from the SecurID RADIUS server.

The transaction state, message, and result is captured into the options dictionary to be utilized during the check_token_list loop. Relevant information is also stored in the Challenge object to track the state of requests.

Changes have been tested with the RSA SecurID Authentication Manager 8.3 and works with softtokens, hard tokens, and ondemand tokens during PIN setup and nexttokencode scenarios.

  •  File: privacyidea/lib/tokens/radiustoken.py:L144-337
  • Please remote this and use the log_with decorator.
  • I've updated this on my end.
  •  File: privacyidea/lib/tokens/radiustoken.py:L144-337
  • The method is_challenge_response only checks if the HTTP request is supposed to be a response to a previous challenge. So there is no need to verify with the remote RADIUS server.
    The verification with the remote RADIUS server is supposed to be located in check_challenge_response.
  • So here's where I need your assistance on design. The reason I'm doing this is due to the fact that the RADIUS server can challenge the user more than once.
    For example:

Send OTP without PIN (initial setup/reset)
< Enter new PIN
Send new PIN
< Re-enter new PIN
Send new PIN again
< Enter next token code + PIN
Send token+PIN
<< Auth-Success
I need the ability to trigger another challenge from within a challenge response. I have basically written a hack/workaround that does a look-ahead within the is_challenge_response to see if the user is being challenged again or not. If they are, I return False from is_challenge_response and True from is_challenge_request.
Is there a better way to do this? I was hoping to not have to make changes to token.py if possible but it appears that may be a necessity.

Merging #1389 into master will decrease coverage by 53.85%.
The diff coverage is 11.03%.
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1389       +/-   ##
===========================================
- Coverage   96.77%   42.91%   -53.86%
===========================================
Files         144      144
Lines       17240    17376      +136
===========================================
- Hits        16684     7457     -9227
- Misses        556     9919     +9363
Impacted Files Coverage Δ
privacyidea/lib/tokens/radiustoken.py 20.23% <11.03%> (-78.03%) ⬇️
privacyidea/lib/smsprovider/SmppSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/ssh.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SipgateSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SmtpSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/pinhandling/base.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/luks.py 0% <0%> (-100%) ⬇️
privacyidea/lib/eventhandler/federationhandler.py 0% <0%> (-98.49%) ⬇️
privacyidea/lib/smsprovider/HttpSMSProvider.py 0% <0%> (-98.27%) ⬇️
privacyidea/lib/eventhandler/counterhandler.py 0% <0%> (-96.78%) ⬇️
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 589c0f7...e08d867. Read the comment docs.

  

droobah added some commits Sep 4, 2018

- added challenges to RADIUS token
- placed logic in is_challenge_response to allow a chain of challenges to occur
- removed dependency on RemoteToken
@cornelinux
Copy link
Member

cornelinux left a comment

Thanks a lot for this pull request!

I admit I probably have not understood it in detail, yet.
So my first comments are mostly kind of design rules in this project.

Would it be also possible to add a test case, so that understanding and verifying gets a bit simpler.
Thanks a lot.

optional = True
required = False

log = logging.getLogger(__name__)


###############################################
class RadiusTokenClass(RemoteTokenClass):
class RadiusTokenClass(TokenClass):

This comment has been minimized.

@cornelinux

cornelinux Jan 21, 2019

Member

What was the reason to change the parent class from RemoteTokenClass (the more specific one) to TokenClass (the totally unspecific)?

This comment has been minimized.

@droobah

droobah Jan 23, 2019

Author

I'm not sure I have any reasoning beyond that I ended up not utilizing any of the underpinnings of the RemoteTokenClass so it felt odd to keep the parent class that way.

Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py
Show resolved Hide resolved privacyidea/lib/tokens/radiustoken.py Outdated
- removed unnecessary logging
- removed unused code in authenticate()
- resolved possible bug arising from uninitialized options dictionary
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #1389 into master will decrease coverage by 53.82%.
The diff coverage is 15.32%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1389       +/-   ##
===========================================
- Coverage   96.77%   42.95%   -53.83%     
===========================================
  Files         144      144               
  Lines       17241    17369      +128     
===========================================
- Hits        16685     7460     -9225     
- Misses        556     9909     +9353
Impacted Files Coverage Δ
privacyidea/lib/tokens/radiustoken.py 22.54% <15.32%> (-75.72%) ⬇️
privacyidea/lib/smsprovider/SmppSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/ssh.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SipgateSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SmtpSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/pinhandling/base.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/luks.py 0% <0%> (-100%) ⬇️
privacyidea/lib/eventhandler/federationhandler.py 0% <0%> (-98.49%) ⬇️
privacyidea/lib/smsprovider/HttpSMSProvider.py 0% <0%> (-98.27%) ⬇️
privacyidea/lib/eventhandler/counterhandler.py 0% <0%> (-96.78%) ⬇️
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e01154d...2793f8b. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #1389 into master will decrease coverage by 53.85%.
The diff coverage is 11.03%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1389       +/-   ##
===========================================
- Coverage   96.77%   42.91%   -53.86%     
===========================================
  Files         144      144               
  Lines       17240    17376      +136     
===========================================
- Hits        16684     7457     -9227     
- Misses        556     9919     +9363
Impacted Files Coverage Δ
privacyidea/lib/tokens/radiustoken.py 20.23% <11.03%> (-78.03%) ⬇️
privacyidea/lib/smsprovider/SmppSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/ssh.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SipgateSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/smsprovider/SmtpSMSProvider.py 0% <0%> (-100%) ⬇️
privacyidea/lib/pinhandling/base.py 0% <0%> (-100%) ⬇️
privacyidea/lib/applications/luks.py 0% <0%> (-100%) ⬇️
privacyidea/lib/eventhandler/federationhandler.py 0% <0%> (-98.49%) ⬇️
privacyidea/lib/smsprovider/HttpSMSProvider.py 0% <0%> (-98.27%) ⬇️
privacyidea/lib/eventhandler/counterhandler.py 0% <0%> (-96.78%) ⬇️
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 589c0f7...e08d867. Read the comment docs.

Merge pull request #1 from privacyidea/master
Resync from upstream
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment