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

Correct barnyard mysql password behavior in Suricata WebUI configuration #379

Merged
merged 3 commits into from
Jul 31, 2017

Conversation

renaudholcombe
Copy link
Contributor

@renaudholcombe renaudholcombe commented Jul 22, 2017

The problem was that upon saving the settings without modifying the mysql password field for barnyard2, the value was being rewritten to the configuration file in it's base64-encoded form.

This change updates the form input type of the field from 'Input' to 'Password' for consistency and applies logic upon post to handle the default behavior of that form element.

This corrects Bug-7716.

@renaudholcombe
Copy link
Contributor Author

@bmeeks8, You were dead on, a variant of this issue existed for suricata. I also crosschecked to verify that neither of the other issues I've looked at exist in their companion pages.

@renaudholcombe renaudholcombe changed the title Correct barnyard mysql password behavior. Correct barnyard mysql password behavior in Suricata WebUI configuration Jul 22, 2017
else
// Because of the base64 encoding/decoding, in the case of a valid value that hasn't changed, it needs to be re-encoded to base64.
if ($_POST['barnyard_dbpwd'] != DMYPWD) unset($natent['barnyard_dbpwd']);
else $natent['barnyard_dbpwd'] = base64_encode($natent['barnyard_dbpwd']);
Copy link
Member

Choose a reason for hiding this comment

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

Please fix style of this entire block respecting the same rules used in this file. It's really hard to read it the way it is.

@bmeeks8 can you check these changes please?

@bmeeks8
Copy link
Contributor

bmeeks8 commented Jul 26, 2017

I am OK with the logic of the submitted changes. They are required to fix improper behavior with the stored password. As the submitter said, the old code (since Bootstrap conversion) was essentially re-encoding and already encoded password upon saving and thus corrupted it.

I do agree the indent style for the added if/then/else statements need fixing.

Bill

@renaudholcombe
Copy link
Contributor Author

@bmeeks8, @rbgarga, sure I can make the formatting changes. I used that style due to the nested nature of the logic from habit and not finding any similarly structured statements within the code.

Are we looking for the entire set of statements to be on a single line or do we want to split them into multiple lines by 'if' statements, keeping the 'else' portions on the next line (if so, what sort of indentation for the nested statements do we want)?

@rbgarga
Copy link
Member

rbgarga commented Jul 27, 2017

@renaudholcombe The rules we use in pfSense are defined in https://doc.pfsense.org/index.php/Developer_Style_Guide#PHP_Specific_Rules

They are basically the same as FreeBSD defines in style(9)

@renaudholcombe
Copy link
Contributor Author

I'm sorry about spilling so much ink on this topic, but @rbgarga, I took a look at the style guide and that suggests the sort of multi-line indentation that I was using initially (in the 'Indent Style' section). I'll make changes to better match the convention within the file using new line indents per nested 'if' statements; would that be the right approach?

@rbgarga
Copy link
Member

rbgarga commented Jul 27, 2017

I know there are code that doesn't follow the rules all around pfSense source, but we try to don't introduce new code like this.

Basically always use brackets { and } and use hard TABs, with size of 8, trying to stay in 80 columns, like:

		if ($_POST['barnyard_dbpwd'] &&
		    $_POST['barnyard_dbpwd'] != DMYPWD) {
			$natent['barnyard_dbpwd'] = base64_encode(
			    $_POST['barnyard_dbpwd']);
		} else {
			/*
			 * Because of the base64 encoding/decoding, in the case
			 * of a valid value that hasn't changed, it needs to be
			 * re-encoded to base64.
			 */
			if ($_POST['barnyard_dbpwd'] != DMYPWD) {
				unset($natent['barnyard_dbpwd']);
			} else {
				$natent['barnyard_dbpwd'] = base64_encode(
				    $natent['barnyard_dbpwd']);
			}
		}

@rbgarga
Copy link
Member

rbgarga commented Jul 28, 2017

Note: Merge combined with #380

@renaudholcombe
Copy link
Contributor Author

Alright @bmeeks8, @rbgarga, I think I've gotten everything sorted out across the pull requests, let me know if there are any changes you'd like. Thanks again for guiding me through this!

@netgate-git-updates netgate-git-updates merged commit 4ae4f31 into pfsense:devel Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants