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

WebUI: Certificate login #559

Closed
wants to merge 2 commits into from
Closed

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Mar 9, 2017

@ghost
Copy link

ghost commented Mar 9, 2017

@pvomacka NACK, see lint errors in travis.

@pvomacka pvomacka force-pushed the cert_log branch 2 times, most recently from 03d32ee to 8bd36a3 Compare March 9, 2017 12:34
Copy link
Member

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

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

I looked mostly at UI, backend will need other reviewer.

this.cert_btn_node = IPA.button({
name: 'cert_auth',
title:"Login using personal certificate",
label: "Smart Card Login",
Copy link
Member

Choose a reason for hiding this comment

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

Title sounds correct, but the title of the button: "Smart Card" is IMO too specific. User can login with cert even without smart card.

@@ -72,9 +78,12 @@ define(['dojo/_base/declare',

user_locked: "The user account you entered is locked. ",

login_url: '/ipa/session/login_x509',
Copy link
Member

Choose a reason for hiding this comment

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

this should be called e.g. x509_url login is too general.

@pvomacka
Copy link
Author

pvomacka commented Mar 9, 2017

@pvoborni thank you for review. Fixed all proposed changes.

GssapiDelegCcachePerms mode:0660 gid:ipaapi
GssapiImpersonate On
NSSVerifyClient require
NSSOCSP On
Copy link

Choose a reason for hiding this comment

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

NSSOCSP can't be specified inside . That's why Travis crashes.

@pvomacka pvomacka force-pushed the cert_log branch 4 times, most recently from 2c34b32 to 0c4c802 Compare March 14, 2017 08:38
Pavel Vomacka added 2 commits March 14, 2017 10:14
Add necessary steps which set SSSD and set SELinux boolean during
installation or upgrade. Also create new endpoint in apache for
login using certificates.

https://pagure.io/freeipa/issue/6225
@ghost
Copy link

ghost commented Mar 14, 2017

LGTM and works.

@ghost ghost added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 14, 2017
@ghost
Copy link

ghost commented Mar 14, 2017

master:

  • 75c592d Support certificate login after installation and upgrade
  • 585547e WebUI: add link to login page which for login using certificate

@ghost ghost closed this Mar 14, 2017
@simo5
Copy link
Contributor

simo5 commented Mar 14, 2017

NACK NACK NACK
Pleas revert the change to the gssproxy template, it undoes half the work done in privilege separation

@simo5
Copy link
Contributor

simo5 commented Mar 14, 2017

You need to wait to get th gssproxy fix I've been developing today and set the minimum gssproxy version to the one with the fix once we get to publish it

@simo5 simo5 reopened this Mar 14, 2017
@simo5 simo5 removed the ack Pull Request approved, can be merged label Mar 14, 2017
Copy link
Contributor

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Remove that line

@pvomacka
Copy link
Author

Removed in #585 once it will be pushed I will close this one again.

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Mar 14, 2017
@MartinBasti
Copy link
Contributor

#585 was pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants