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 <de...@ikedam.jp>: >>> >>> 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 jenkinsci-de...@googlegroups.com. >>> 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 jenkinsci-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
