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

[JENKINS-73812] Adding minimum password length for FIPS compliance #200

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[JENKINS-73812] Adding minimum password length for FIPS compliance #200

wants to merge 13 commits into from
+612 −11

Conversation

nevingeorgesunny
Copy link
Contributor

@nevingeorgesunny nevingeorgesunny commented Sep 25, 2024 •
edited
Loading

In the domain section of active directory
if you enter a short password , and the focus leaves from the field we get this message
image

is the user still clicks on on test domain it thows angry Jenkins error

image

and if the user clicks on save , this same error appears
image

if the user click on apply a popup with error shows
image

image

@nevingeorgesunny nevingeorgesunny changed the title Adding minimum password length for FIPS compliance [BEE-51816] Adding minimum password length for FIPS compliance Sep 25, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java Fixed Show fixed Hide fixed
src/main/java/hudson/plugins/active_directory/ActiveDirectoryDomain.java
* Displays an error message if the provided password is less than 14 characters
* while in FIPS mode. This message is triggered when the bindPassword field loses focus.
*/
public FormValidation doCheckBindPassword(@QueryParameter String bindPassword) {

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doCheckBindPassword
@nevingeorgesunny nevingeorgesunny changed the title [BEE-51816] Adding minimum password length for FIPS compliance [BEE-52185] Adding minimum password length for FIPS compliance Sep 25, 2024
@nevingeorgesunny nevingeorgesunny changed the title [BEE-52185] Adding minimum password length for FIPS compliance Adding minimum password length for FIPS compliance Sep 25, 2024
@nevingeorgesunny nevingeorgesunny changed the title Adding minimum password length for FIPS compliance [JENKINS-73812] Adding minimum password length for FIPS compliance Sep 25, 2024
@nevingeorgesunny nevingeorgesunny marked this pull request as ready for review September 30, 2024 06:38
rsandell
rsandell requested changes Sep 30, 2024
View reviewed changes
Copy link
Member

@rsandell rsandell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would like to see some more integration type tests. Testing that calling a constructor fails is all well an good, but is not very future proof when the class evolves.
Testing that the validation error shows up on the configuration page and that it fails when the actual form is submitted is better.
Also a test for a bad JCasC configuration should also be done.

@nevingeorgesunny
Copy link
Contributor Author

I would like to see some more integration type tests. Testing that calling a constructor fails is all well an good, but is not very future proof when the class evolves. Testing that the validation error shows up on the configuration page and that it fails when the actual form is submitted is better. Also a test for a bad JCasC configuration should also be done.
all the following test are run in FIPS mode

I have added these Integration test

  1. Using local data, small password is set , click on "test Domain" this will throw angry Jenkins error
  2. Using local data , small password is set , click on "Save"this will throw angry Jenkins error
  3. Using local data , small password is set , click on "Apply"this will throw angry Jenkins error
  4. Using local data , valid password is set , click on "Apply" this will return 200 then I update the password to small then i click on "Apply "this will throw angry Jenkins error

I have added some JCasC tests

  1. Setting up a Jenkins with CasC YAML containing small password, this will throw error
  2. Setting up a Jenkins with CasC YAML containing valid password, this should work

fcojfernandez
fcojfernandez reviewed Oct 1, 2024
View reviewed changes
src/test/java/hudson/plugins/active_directory/ActiveDirectoryDomainIntegrationTest.java Outdated
Comment on lines 118 to 148
/**
* Tests the behavior of the "Test Domain" button when a short password is configured.
*
* <p>For the preconfigured value, the password is "small" in the local data.
* When the "Test Domain" button is clicked, the page should display an error message
* indicating that the password is too short, along with an "angry Jenkins" error message.</p>
*
*/
// @LocalData
// @Test
// public void testActiveDirectoryDomainTestDomainButtonClickWithShortPassword() throws Exception {
// JenkinsRule.WebClient webClient = jenkins.createWebClient();
// // Navigate to the configuration page
// HtmlPage configPage = webClient.goTo("configureSecurity");
// HtmlForm form = configPage.getFormByName("config");
//
// //Check that the password is too short message is present
// assertTrue(form.asNormalizedText().contains(Messages.passwordTooShortFIPS()));
//
// // Click the "Test Domain" button
// HtmlPage resultPage = getButtonByText(form, "Test Domain").click();
//
// webClient.waitForBackgroundJavaScript(2000); // Wait for up to 5 seconds
//
// String responseContent = resultPage.asNormalizedText();
// // Assert that the error message is present in the page content
// assertTrue(responseContent.contains("A problem occurred while processing the request"));
//
// //Check that the password is too short message is present
// assertTrue(responseContent.contains(Messages.passwordTooShortFIPS()));
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

why adding a commented test? please either fix it or remove it, but don't add a commented method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was failing on the pipeline, but it was working. locally . I just want to check if it only this test that had this issues , I Will uncomment it once I fix the issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep getting the error [Test Domain] not found , but this does not happen locally , am i configuring something wrong ?

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

@olamy olamy olamy requested changes

@PereBueno PereBueno PereBueno left review comments

@fcojfernandez fcojfernandez fcojfernandez left review comments

@rsandell rsandell rsandell requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nevingeorgesunny @olamy @rsandell @PereBueno @fcojfernandez