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.

Sign up for GitHub

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

Jump to bottom

Rolling back system token OTP logic #219

Merged
merged 2 commits into from
Nov 21, 2017
Merged

Rolling back system token OTP logic #219

merged 2 commits into from
Nov 21, 2017

Conversation

abellotti
Copy link
Member

  • This was breaking central admin functionality.
  • Preferring a short-term token of 30 seconds instead.

/cc @carbonin @gtanzillo

@carbonin
Copy link
Member

carbonin commented Nov 20, 2017 •
edited
Loading

This reverts #178 and also changes the token lifetime from 5.minutes to 30.seconds

It should also resolve https://bugzilla.redhat.com/show_bug.cgi?id=1514607

imtayadeway
imtayadeway suggested changes Nov 20, 2017
View reviewed changes
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, but shouldn't this use git revert and a separate commit for the ttl change?

@abellotti
Revert "Merge pull request ManageIQ#178 from abellotti/api_onetime_sy…
28b92fa 
…stem_token"

This reverts commit e8c018b, reversing
changes made to 9a2767c.
@abellotti
Updating the system token ttl to 30 seconds - down from 5 minutes.
21c3572 
It was decided to have the system token ttl have a short lifespan of a
minute or less to minimize the window for token snooping and possible DOS attacks.
imtayadeway
imtayadeway approved these changes Nov 20, 2017
View reviewed changes
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@miq-bot
Copy link
Member

miq-bot commented Nov 20, 2017

Checked commits abellotti/manageiq-api@28b92fa~...21c3572 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@chrisarcand chrisarcand merged commit f80b70b into ManageIQ:master Nov 21, 2017
@chrisarcand chrisarcand assigned chrisarcand and unassigned Fryguy Nov 21, 2017
simaishi pushed a commit that referenced this pull request Nov 21, 2017
@chrisarcand @simaishi
Merge pull request #219 from abellotti/otp_to_stt_systoken
99b25ce 
Rolling back system token OTP logic
(cherry picked from commit f80b70b)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 99b25ce2229dde96c8834bcc1c06c66e4a8be596
Author: Chris Arcand <chrisarcand@users.noreply.github.com>
Date:   Mon Nov 20 18:10:36 2017 -0600

    Merge pull request #219 from abellotti/otp_to_stt_systoken
    
    Rolling back system token OTP logic
    (cherry picked from commit f80b70b3027ba8a7f409d8cf64daba552b528a22)

@abellotti abellotti deleted the otp_to_stt_systoken branch November 22, 2017 14:00
@JPrause
Copy link
Member

JPrause commented Apr 19, 2018

@abellotti can you review to see if this can be backported to fine branch. If yes, can you add the fine/yes label. See for ref: https://bugzilla.redhat.com/show_bug.cgi?id=1552267

@abellotti
Copy link
Member Author

fine branch did not have the OTP logic, so no need to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

@imtayadeway imtayadeway imtayadeway approved these changes

@chrisarcand chrisarcand chrisarcand approved these changes

Assignees
No one assigned
Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@abellotti @carbonin @miq-bot @simaishi @JPrause @gtanzillo @imtayadeway @chrisarcand @Fryguy