Ticket #10959 (closed PLIP: fixed)

Opened 4 years ago

Last modified 20 months ago

API for password validation policy

Reported by: djay Owned by:
Priority: minor Milestone: 4.3
Component: Backend (Python) Version: 4.1
Keywords: Cc: ggozad, plip-advisories@…, djay, mitchell

Description (last modified by djay) (diff)

Proposer: Dylan Jay Seconder: Ken Wasetis

Motivation

Individual site policies might call for different levels of passwords strengths. Currently there is no api to easily integrate alternative password strength rules into plone.

Assumptions

This PLIP is for api only and won't change the current strength default plone uses for passwords. However because we need to support adding users without passwords and because setting initial password that meet all rules of all plugins is hard, we assume we will change the policy of generating a 5 char password. Instead we will set an very long random password no one will ever see since the password reset tool is used to send a welcome email with a link to set a new password. We'll also assume there could be multiple plugins working at once. Each plugin will return a set of error messages which will be already translated.

Proposal & Implementation

PAS already has a plugins for validating user properties. This would be an obvious choice. The Products.PasswordStrength plugin is implemented as a PAS plugin. If desired an more z3 api could be created instead. A much longer stronger password will be generated that is likely to pass any validation. Since this is never sent it doesn't need to exactly match any policy.

Deliverables

Changes to plone.app.users to call out to api to validate the password. i18n is the responsibility of the password validation plugin. Documentation needs to be created on creating a password validation plugin. Move the current default 5 char validation to a plugin of its own instead of in plone.app.users. Probably in Products.PlonePAS. A new workflow for adding new users without setting a password and without sending a clear text password.

Risks

  • We will have to join i18n strings togeather in an i18n way cause we are getting multiple errors from different plugins.

Participants

Dylan Jay - djay.

Progress

Similar changes have been done for plone3.x as part of Products.PasswordStrength. There would be migrated to the new plone4 implementation.

Change History

comment:1 Changed 4 years ago by djay

  • Description modified (diff)

comment:2 follow-up: ↓ 8 Changed 4 years ago by ldr

Can you confirm that all that is required here is changing plone.app.users to use an existing PAS api to check password validity?

comment:3 follow-up: ↓ 7 Changed 4 years ago by esteele

Your PLIP has been accepted for consideration for Plone 4.1.

Framework Team voting on this PLIP was: Alec +1 Craig +1 Elizabeth +0 Laurence +1 Martijn +1 Matthew +1 Rob +1 Ross --

The initial implementation deadline for your PLIP is October 1st, 2010. The Framework Team would certainly appreciate you finishing beforehand so that they may begin evaluating it as soon as possible. Announce its readiness here once your implementation is ready for review.

comment:4 Changed 4 years ago by esteele

  • Summary changed from PLIP: API for password validation policy to API for password validation policy

comment:5 follow-up: ↓ 9 Changed 4 years ago by ggozad

  • Cc ggozad added

I had done some work for ENISA on a generic password policy product that would do password complexity, aging and expiration as well as keep a password history so that the same password is not reused. Let me know if you need help I would be happy to adapt it and contribute.

comment:6 Changed 4 years ago by cah190

  • Cc plip-advisories@… added

comment:7 in reply to: ↑ 3 Changed 4 years ago by djay

Replying to esteele:

Your PLIP has been accepted for consideration for Plone 4.1.

Framework Team voting on this PLIP was: Alec +1 Craig +1 Elizabeth +0 Laurence +1 Martijn +1 Matthew +1 Rob +1 Ross --

The initial implementation deadline for your PLIP is October 1st, 2010. The Framework Team would certainly appreciate you finishing beforehand so that they may begin evaluating it as soon as possible. Announce its readiness here once your implementation is ready for review.

I'm really sorry but it appears that I've got no email notifications that this PLIP was accepted. I will endeavor to work on this weekend. I hope this will still enable it's inclusion.

comment:8 in reply to: ↑ 2 Changed 4 years ago by djay

Replying to ldr:

Can you confirm that all that is required here is changing plone.app.users to use an existing PAS api to check password validity?

Yes. It's simply a few changes to where the password validity is checked which will then look up an existing PAS api. If no such PAS plugin exists then the existing 5 char password rules will be enforced.

comment:9 in reply to: ↑ 5 Changed 4 years ago by djay

Replying to ggozad:

I had done some work for ENISA on a generic password policy product that would do password complexity, aging and expiration as well as keep a password history so that the same password is not reused. Let me know if you need help I would be happy to adapt it and contribute.

I'd love to discuss this. Please contact me. The scope of this PLIP is just to put in place hooks to make it easy to put in 3rd party plugins with validation rules. That would cover password complexity and history but wouldn't handle expiration and aging. Did it involve patching core code to enable aging and expiration?

comment:10 Changed 4 years ago by djay

  • Cc djay added

comment:11 Changed 3 years ago by esteele

  • Milestone changed from 4.1 to Future

comment:12 Changed 3 years ago by rossp

  • Status changed from new to closed
  • Resolution set to wontfix

PLEASE READ THIS AND RE-OPEN VALID PLIPS!

As we launch the new PLIP process we'd like to see which PLIPs:

  • are still appropriate/needed
  • still have owners/proposers/champions
  • still have available implementers

If this PLIP should still be considered for future releases of Plone please do re-open this ticket and assign an appropriate milestone. If it should be considered for the next release of Plone, use the 4.2 milestone. Also be sure to update the PLIP description, requester, owner, etc. and include a comment detailing recent progress and new plans. We will use all these details in the new continuous PLIP process.

comment:13 Changed 3 years ago by djay

  • Status changed from closed to reopened
  • Resolution wontfix deleted
  • Milestone changed from Future to 4.2

Implementation is done, just finishing tests remain.

comment:14 follow-up: ↓ 15 Changed 3 years ago by eleddy

FWT approved for 4.2 with a couple notes:

  • Tests must be included
  • 1 external review must be from the security team
  • matthewwilkes will be your framework team contact for this

Please note the new process needs two external reviews. When you are ready for the FWT to do an integration review please update status to "please review"

Thanks!

comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 3 years ago by rossp

Replying to eleddy:

Thanks for following up. The deadline for feature freeze is June 30th, so we need to start doing implementation reviews as soon as possible. Have tests been added? Has an the security team reviewed this? IOW, is this ready for implementation review?

Also, I am not the FWT contact for this. :-)

comment:16 in reply to: ↑ 15 Changed 3 years ago by djay

Replying to rossp:

Replying to eleddy:

Thanks for following up. The deadline for feature freeze is June 30th, so we need to start doing implementation reviews as soon as possible. Have tests been added? Has an the security team reviewed this? IOW, is this ready for implementation review?

Yes there are tests and the implementation is there but I haven't had a chance to run it against the 4.2 branch as yet. There is one issue with the current implementation. If you use additional password validation rules then automatically generated passwords can fail to be valid. I need to investigate if there is a PAS hook for generating passwords so plugins can do this as well.

Also, I am not the FWT contact for this. :-)

comment:17 Changed 3 years ago by rossp

Since we need to have a feature freeze by June 30th, we need implementations finished by next week's framework team meeting on Tuesday, June 14th. IOW, implementation will need to be finished on Monday, June 13th.

Will you be able to have the implementation done by then?

comment:18 Changed 3 years ago by djay

The implementation is done and works. The tests are done and should work but don't. Something is going funny whereby I add a PAS plugin in the test code but it disappears when accessed later. I'm trying to work out how but no luck so far.

comment:19 Changed 3 years ago by rossp

Ok, FWT is gonna treat this implementation complete and ready for review.

comment:20 Changed 3 years ago by rossp

(In [50580]) Review PAS password validation API PLIP. Refs #10959.

Not ready yet.

comment:21 follow-up: ↓ 22 Changed 3 years ago by djay

Any more detail on exactly what is missing? (other than working out why the tests don't work which has already been mentioned?)

comment:22 in reply to: ↑ 21 Changed 3 years ago by rossp

Replying to djay:

Any more detail on exactly what is missing? (other than working out why the tests don't work which has already been mentioned?)

Details are in the review added in that commit.

comment:23 follow-up: ↓ 24 Changed 3 years ago by djay

Sorry, missed that. new to the process.

Yes you are right. I've realised I'm missing part of the implementation. I should have that checked in tomorrow with working tests and will include a version of Products.PasswordStrength in the buildout for testing purposes. If this change is implementation is too late I understand.

comment:24 in reply to: ↑ 23 Changed 3 years ago by rossp

Replying to djay:

Sorry, missed that. new to the process.

Yes you are right. I've realised I'm missing part of the implementation. I should have that checked in tomorrow with working tests and will include a version of Products.PasswordStrength in the buildout for testing purposes. If this change is implementation is too late I understand.

I'm fine with a delay till tomorrow and I'm guessing the rest of the FWT will be too.

Thanks for including Products.PasswordStrength. Can you also include (maybe in a brief comment here) how to use it for testing?

comment:25 follow-up: ↓ 26 Changed 3 years ago by djay

A new implementation has been completed including tests.

You can test it by installing Products.PasswordStrength (included in buildout). Once activated it will enforce a much stricter password policy. You'll need to go to acl_users/password_strength_plugin to adjust the policy. Also I've added support for password generation plugin.

comment:26 in reply to: ↑ 25 Changed 3 years ago by rossp

Replying to djay:

A new implementation has been completed including tests.

You can test it by installing Products.PasswordStrength (included in buildout). Once activated it will enforce a much stricter password policy. You'll need to go to acl_users/password_strength_plugin to adjust the policy. Also I've added support for password generation plugin.

Can you please provide more details of how to set up a simple validation criterion using Products.PasswordStrength? It's very slim on the docs so it's hard to figure out how to test this PLIP. Thanks!

comment:27 Changed 3 years ago by djay

rossp: I misspoke. There is no need to setup a new policy. PasswordStrength comes with one by default. However the one thing it doesn't do as yet is install itself for password generation yet. To do that go to the plugin in acl_users and activate it for as a "Properties Plugin".

ggozad: Tests now all pass. I've also added documentation which shows how to add your own plugins in tests/plugins.txt. Plugins will be able to do things like use password history. Password expiry is out of scope of this PLIP. Expiry is a good idea however and is possible by hooking into both PAS authentication and the Plone login screens. Perhaps another PLIP? or if you have code already for this?

comment:28 follow-up: ↓ 31 Changed 3 years ago by ggozad

(In [50864]) Updated review for Password validation API. Refs #10959

comment:29 Changed 3 years ago by ggozad

Hey Dylan, The PLIP was discussed yesterday on the FWT meeting. The following concerns were raised:

  • We would like to see this done at the PAS level (for password generation for instance). In the control panel and p.a.users we should see as little as possible, essentially just UI.
  • Usage of user properties looks insecure.
  • A mechanism should be implemented on the plugin level to guarantee that initial password generation does not fail.

comment:30 Changed 3 years ago by djay

discussed with ggozad the idea of creating new PAS plugin interfaces in PAS itself which I'll have a look at.

However, another solution I've been considering is not setting a password at all. Users would have a disabled state and we use the password reset tool instead. Would like feedback on that.

comment:31 in reply to: ↑ 28 Changed 3 years ago by rossp

  • Milestone changed from 4.2 to 4.3

Replying to ggozad:

(In [50864]) Updated review for Password validation API. Refs #10959

The FWT agrees with ggozad's review above and suggests targeting this for 4.3 with an implementation that addresses that feedback.

comment:32 Changed 3 years ago by rossp

The FWT has wrapped up work on 4.2 and can start work on 4.3 whenever we have PLIPs to review. So can you as proposers or implementers please check in on your PLIPs and let us know what the status is and when we can expect issues to be addressed and implementations complete so we can review them for merge in 4.3.

comment:33 Changed 3 years ago by djay

  • Description modified (diff)

added in changing policy of sending passwords in an email. Now use password reset if want to not choose a password.

comment:34 Changed 3 years ago by djay

  • Description modified (diff)

changed description to reflect that passwords aren't sent so therefore no need for a password generation plugin api. Just need to make the generated passwords much stronger.

comment:35 Changed 2 years ago by djay

Hi,

I don't fully understand previous comments "We would like to see this done at the PAS level "

Just want to confirm that what I'm going to code is going to suitable this time.

  1. Move the 5char password policy to its own password validation plugin in Products.PlonePAS
  2. remove all the code I added for a password generator plugin
  3. Make all auto generated passwords extremely long since its been determined that at no time does a new user see the autogenerated password. As long as the password is unguessable we should be fine.

Is this ok?

comment:36 Changed 2 years ago by rossp

@djay I think I missed that meeting so ggozad will have to clarify.

comment:37 Changed 2 years ago by ggozad

Hey! I think you are in the right direction. As long as the points addressed in the reviews are answered and acted upon I am fine with it. When you are ready and have something for review let us know.

comment:38 Changed 2 years ago by rossp

@djay Did you see this response to your PLIP? Can you comment on the ticket as to whether you'll be able to finish the implementation for review for Plone 4.3? Implementations have to be finished by the first week of January.

comment:39 follow-up: ↓ 40 Changed 2 years ago by djay

Yes. I intend to meet the implementation date. However I'd like to avoid the situation last time whereby I fully implemented a solution where the design was latter rejected. Is there a standard process for reviewing during implementation? I DM ggozad to see if he would review as I went but I'm not sure if thats correct or not.

comment:40 in reply to: ↑ 39 Changed 2 years ago by esteele

Replying to djay:

Yes. I intend to meet the implementation date. However I'd like to avoid the situation last time whereby I fully implemented a solution where the design was latter rejected. Is there a standard process for reviewing during implementation? I DM ggozad to see if he would review as I went but I'm not sure if thats correct or not.

Part of the way the FWT is trying to do things now is to make it more of a continuous review process. Ask questions, either here or on the Framework Team mailing list and they'll give feedback. If not, ping me and I'll make sure someone takes a look at it.

ggozad is taking a break from the FWT for a bit, so I'm unsure whether he's able to respond to any particular questions right now. Ross, are you the new FWT champion for this PLIP?

comment:41 Changed 2 years ago by djay

Hi,

I wanted to get some feedback that I'm heading in right direction before I finish this work

 https://github.com/plone/Products.PlonePAS/commit/5966fa4ec68b73dd036d6ebbaf1c8ff99e3bc316

 https://github.com/plone/Products.CMFPlone/commit/de366e67f43e1a4e314eb57ed30a4b9aa2ec84d6

TODO: Make tests pass. Move changes to Products/PlonePAS/plugins/property.py into it's own defaultplonepassword policy object so it's clear it's optional. Upgrade steps etc.

Version 0, edited 2 years ago by djay (next)

comment:42 Changed 2 years ago by djay

I've pushed an update plip cfg with a working set of initial tests.

comment:43 Changed 2 years ago by rossp

I've  updated my review and reviewed the code in a  pull request I started for the purpose of reviewing the implementation.

Summary, I'm still -1 for merge.

comment:44 Changed 2 years ago by rossp

Oops, forgot to mention, I'm -1 for merge but if I recall, there still time to address the issues for 4.3.

comment:45 Changed 2 years ago by rossp

I'm unable to be an  effective champion for this PLIP. I think it's a great idea, though. Anyone want to take over?

comment:46 Changed 2 years ago by djay

I'm happy for someone to take over the implementation too.

comment:47 Changed 2 years ago by eleddy

  • Cc mitchell added
  • Status changed from reopened to confirmed
  • Version set to 4.1
  • Component changed from Unknown to Backend (Python)

Okay guys - there is some overlap happening here. We have two plips that are basically the same, one that was partially implemented and one that is proposed. Any chance you (djay and mitchell) can get together, put together 1 plip and we can go from there???

Refs #10959, #12521

comment:48 Changed 2 years ago by djay

This PLIP is fully functional just a little more work to make the tests pass.

No API is needed for custom password generation as generated passwords are no longer given out to users.

The sticking point is that I keep getting different advise on how to implement this API. It's currently implemented as a PAS User Property validation plugin which seemed the most logical as it didn't require any new API's to be created. PAS is simple to understand and already extensively used in Plone. The current implementation returns both the description of what kind of password is needed and specific validation errors from the same function call.

It's really up to the FWT to decide what kind of API they will accept. I'm happy for mitchell to reimplement this to provide a different style API if that's the preferred solution.

comment:49 Changed 2 years ago by mitchell

Sounds good to me. I haven't made much progress since the proposal, so happy to defer to Dylan.

comment:50 Changed 2 years ago by djay

it would be useful to get some feedback from the FWT if they want me to proceed with this PLIP or not. If they do, then I'll need a mentor who can give me advise on how to make it acceptable to the FWT.

comment:51 Changed 20 months ago by esteele

  • Status changed from confirmed to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.