Oleg, thanks for comments. I appreciate comments for this plugin especially about security issues, as I know this plugin can easily cause security vulnerabilities by any design mistakes.
I want to explain what you pointed does not result security vulnerabilities: > - The real authentication checking code is commented, so the validation > will be passed for every user (see > SpecificUserAuthorizationStrategy.java:375 ) > - [hidden by the previous bug] - In "Specific Users Strategy" you check > the password multiple times after the each change of the input field. It > may lead to locking of user's account due to limited number of > authentication attempts The commented codes are in doCheckPassword, which just does real-time form validation. This validation does nothing when the form is submitted. That code is commented out for some reasons including what you pointed. I should add comments explaining that. The real check in saving a configuration is done in Descriptor#newInstance. Please see what happens if you enter an invalid user authentication tokens: the submission will fail with following message: Failed to authenticate the user specified to run builds with its authorization. Please check User ID and Password is valid. Though I know that is not so convenient for users, that is the only way I can ensure works in any web browsers. I tried another approach, but gave up. If you are interested, please have a look at https://github.com/ikedam/authorize-project/tree/feature/ConfigureHook. That displays a popup to require a password when a user submitted a form, and he or she cannot proceed till entering the correct one. > - Passwords are being transferred as a plain text inside AJAX request > - doCheckPassword() is being triggered even if "Configure Build > Authorization" is unchecked I think these are rather issues of Jenkins core (or every web applications). Passwords are also transferred in a plain text when logging in. That also the case for almost all web applications, and HTTPS should be used. Form validations are performed for any disabled fields. And I think that does not cause this plugin to harm securities. > - [MAJOR] - you use User.get(String) to retrieve users. This method > creates new users on-demand, so your null checks won't work in any case. In > addition, this function may lead to creation of new users if admin uses > "Specific User" strategy You are right. I should use not User.get(String), but User.get(String, boolean, Map). http://javadoc.jenkins-ci.org/hudson/model/User.html#get%28java.lang.String,%20boolean,%20java.util.Map%29 > - Matrix Projects are not handled (Unfortunately, it is applicable to > almost every new plugin) I agree. I should retrieve the project using AbstractProject.getRootProject. And I should add test cases for MatrixProject. > - hetero-radio selector overloads web UI in the case of big number of > strategies. I suppose that dropdownDescriptor selector would be preferable I'm not sure. I doubt there will be so more strategies than now. Regards, ikedam On Wednesday, November 27, 2013 5:35:58 PM UTC+9, Oleg Nenashev wrote: > > Hello, > > Thanks a lot for the plugin. > I've spent some time for its evaluation and found some issues in the > implementation, > You can find my comments below. I'll also create JIRA issues today. > > "Specific Users Strategy" has critical and security vulnerabilities issues: > > - The real authentication checking code is commented, so the > validation will be passed for every user (see > SpecificUserAuthorizationStrategy.java:375 ) > - [hidden by the previous bug] - In "Specific Users Strategy" you > check the password multiple times after the each change of the input > field. > It may lead to locking of user's account due to limited number of > authentication attempts > - Passwords are being transferred as a plain text inside AJAX request > - doCheckPassword() is being triggered even if "Configure Build > Authorization" is unchecked > > Other comments: > > - [MAJOR] - you use User.get(String) to retrieve users. This method > creates new users on-demand, so your null checks won't work in any case. > In > addition, this function may lead to creation of new users if admin uses > "Specific User" strategy > - Matrix Projects are not handled (Unfortunately, it is applicable to > almost every new plugin) > - hetero-radio selector overloads web UI in the case of big number of > strategies. I suppose that dropdownDescriptor selector would be preferable > > Best regards, > Oleg Nenashev > > > воскресенье, 24 ноября 2013 г., 10:32:14 UTC+4 пользователь Ikedam написал: >> >> I successfully released my plugin. >> Thanks! >> >> ikedam >> >> On Saturday, November 23, 2013 7:52:40 PM UTC+9, Ullrich Hafner wrote: >>> >>> Created >>> https://github.com/jenkinsci/authorize-project-plugin<https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2Fjenkinsci%2Fauthorize-project-plugin&sa=D&sntz=1&usg=AFQjCNG-JIWRQgpGtIJBP4HqbTixDxeWmQ> >>> >>> Ulli >>> >>> Am 23.11.2013 um 03:58 schrieb Ikedam <[email protected]>: >>> >>> Hello. >>> >>> I want my plugin hosted in >>> https://github.com/jenkinsci/<https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2Fjenkinsci%2F&sa=D&sntz=1&usg=AFQjCNHwWz3Exrk5vSGYEREtUX5IiEU87Q>. >>> https://github.com/ikedam/authorize-project/<https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2Fikedam%2Fauthorize-project%2F&sa=D&sntz=1&usg=AFQjCNEG3KzTzr1yJid9dhtGBUzCFYWSmA> >>> >>> My Github account is "ikedam". >>> >>> This plugin provides an implementation of QueueItemAuthenticator, which >>> is introduced in Jenkins 1.520. >>> >>> http://javadoc.jenkins-ci.org/jenkins/security/QueueItemAuthenticator.html<http://www.google.com/url?q=http%3A%2F%2Fjavadoc.jenkins-ci.org%2Fjenkins%2Fsecurity%2FQueueItemAuthenticator.html&sa=D&sntz=1&usg=AFQjCNHINEalIrBAzbgfNgC_neb7kEPbnw> >>> >>> Users can configure a project to run its builds with authorization of a >>> specified user. >>> * Run as anonymous >>> * Run as a user who triggered the build >>> * Run as a user specified with its user ID. >>> >>> Regards, >>> ikedam >>> >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Jenkins Developers" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> For more options, visit https://groups.google.com/groups/opt_out. >>> >>> >>> -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/groups/opt_out.
