Unhide commented out options variables in john.conf

[jfoug]

We should try to NOT have a bunch of commented option variables in john.conf. We have the john option of --list-parameters:Options which dumps a list of what the current settings are. If lines are commented out, then they do NOT show up in this dump, EVEN THOUGH there is default value for this item(s).

So:

# Un-commenting this stops Single mode from re-testing guessed plaintexts
# with all other salts.
#SingleRetestGuessed = N

But should be

# Change to 'N' to stop Single mode from re-testing guessed plaintexts
# with all other salts.
SingleRetestGuessed = Y

These commented out lines in the Options section should all be looked at, and (if possible), uncommented with proper default setting set, and the comment changed. This will allow the option to show up if the --list=parameters:Options

Yes, a bit off-topic here, but I wanted this to be talked about. If it is deemed 'a good thing', then we can easily create a issue to hang it on.

NOTE, this needs to be done carefully. We need to look at how each variable is used, and properly set the variable BASED upon how it's defaults appear to be. It can be opposite of how the commented out variable is (as in the SingleRetestGuessed instance). But there are others where simply removing the comment and leaving it set as is, is correct.

These are the current commented variables:

#WordlistRules = Wordlist
#DefaultIncrementalUTF8 = UTF8
#LogDateFormat = %Y-%m-%dT%H:%M:%S%z
#LogDateFormatUTC = Y
#LogDateStderrFormat = %b %d %H:%M:%S
#LogFilePermissions = 0600
#PotFilePermissions = 0600
#IgnoreChmodErrors = N
#SingleRetestGuessed = N
#SessionFileProtect = Named
#LogFileProtect = Named
#DefaultInternalCodepage = ISO-8859-1

[frank-dittrich]

The current --list-parameters:section has been implemented with scripts in mind.
For end users, an enhanced version might be needed.

That enhanced version should ideally

• print any preceding comments from the config file
• print the parameter names in CamelCase (like in the config file)
• indicate whether the current setting is the default setting or a setting which has been adjusted by the user

[jfoug]

indicate whether the current setting is the default setting or a setting which has been adjusted by the user

Very important, AND useful, for sure! But this issue is simply to adjust the john.conf file itself, removing the comments (effectively) for items in the [Options] section.

I do really like your ideas. but you are right (and really 'should' write a task for this)

The current --list-parameters:section has been implemented with scripts in mind.
For end users, an enhanced version might be needed.

[frank-dittrich]

Since the users are not longer supposed to change john.conf to adjust config settings, we might just assume that any value in a Section name without the Local: part is the default value, while
any value in a section named [Local:<something>] has been adjusted by the user.

Alternatively, we could change the way config variables have to be defined, although this would mean another deviation from core.
(E.g., instead of just parsing a section for a parameter value, it would be required to first somehow advertise or register this new parameter, the default value and any other possible values.
This would make listing all known parameters and their possible values easier. It would also make it easier to detect certain config errors. But I am not sure this it worth the additional effort.)

@jfoug
While I was adding this comment, I noticed your reply.
I will create a new issue for this, but probably not today.

[magnumripper]

Since the users are not longer supposed to change john.conf to adjust config settings, we might just assume that any value in a Section name without the Local: part is the default value

I disagree. Nothing says they should not edit that file, and nothing should say so. Our john-local.conf hack is of much more use for ourselves than for people happy with running latest release with an update or two per decade, at best.

Let's not over engineer this. We fixes the commented-out options and we is done with it. We have lots and lots of issues to fix rather than adding code with shady assumptions.

[magnumripper]

These are the current commented variables:

#WordlistRules = Wordlist
#DefaultIncrementalUTF8 = UTF8
#LogDateFormat = %Y-%m-%dT%H:%M:%S%z
#LogDateFormatUTC = Y
#LogDateStderrFormat = %b %d %H:%M:%S
#LogFilePermissions = 0600
#PotFilePermissions = 0600
#IgnoreChmodErrors = N
#SingleRetestGuessed = N
#SessionFileProtect = Named
#LogFileProtect = Named
#DefaultInternalCodepage = ISO-8859-1

Some of them (eg. WordlistRules and LogDateFormat) can only be uncommented with them set to the empty string. Not sure how to handle them. Perhaps like this?

 # optional add a date timestamp in front of every logged line.
 # the default is no timestamp logging. See the docs for
 # strftime for more information:
 # http://en.cppreference.com/w/c/chrono/strftime
 #
 # 2016-02-20T22:35:38+01:00 would be %Y-%m-%dT%H:%M:%S%z
 # Feb 20 22:35:38           would be %b %d %H:%M:%S
 #LogDateFormat = %Y-%m-%dT%H:%M:%S%z
+LogDateFormat =

Then we need to assure that is the same as having them commented out. For eg. LogDateFormat I think we might get into trouble. Code looks like this:

        LogDateFormat = cfg_get_param(SECTION_OPTIONS, NULL,
                                    "LogDateFormat");
(...)
        if (LogDateFormat) {
                struct tm *t_m;
                char Buf[128];
                time_t t;
(...)

We probably (I haven't tested it) will end up with a pointer to a null string (as opposed to a null pointer) so the test should be if (LogDateFormat && *LogDateFormat). So this change isn't quite as trivial as we thought.

[jfoug]

I would like to start on this, but it will clash with 5c76467 so I will hold up on this one until that is completed.

[magnumripper]

It wouldn't clash as long as you don't change SingleRetestGuessed. Anyway it's merged now.

[jfoug]

It wouldn't clash as long as you don't change SingleRetestGuessed. Anyway it's merged now. I see merged, and am getting started on this.  Some will be very trivial. Some will take a bit of research.  It will be in a branch, so others can comment, especially since comments are changing, and others may have better verbiage than I initially produce.

[jfoug]

Perhaps like this?

 # optional add a date timestamp in front of every logged line.
 # the default is no timestamp logging. See the docs for
 # strftime for more information:
 # http://en.cppreference.com/w/c/chrono/strftime
 #
 # 2016-02-20T22:35:38+01:00 would be %Y-%m-%dT%H:%M:%S%z
 # Feb 20 22:35:38           would be %b %d %H:%M:%S
 #LogDateFormat = %Y-%m-%dT%H:%M:%S%z
+LogDateFormat =

My idea on that was exactly the same, minus the #LogDateFormat = comment line. So it would look like this:

 # optional add a date timestamp in front of every logged line.
 # the default is no timestamp logging. See the docs for
 # strftime for more information:
 # http://en.cppreference.com/w/c/chrono/strftime
 #
 # 2016-02-20T22:35:38+01:00 would be %Y-%m-%dT%H:%M:%S%z
 # Feb 20 22:35:38           would be %b %d %H:%M:%S
+LogDateFormat =

[jfoug]

Ok, need a bit of insite:

static void ldr_set_encoding(struct fmt_main *format)
{
	// .....

	/* john.conf alternative for --internal-codepage */
	if (options.flags &
	    (FLG_RULES | FLG_SINGLE_CHK | FLG_BATCH_CHK | FLG_MASK_CHK))
	if ((!options.target_enc || options.target_enc == UTF_8) &&
	    !options.internal_cp) {
		if (!(options.internal_cp =
			cp_name2id(cfg_get_param(SECTION_OPTIONS, NULL,
			                         "DefaultInternalCodepage"))))
			options.internal_cp =
			    cp_name2id(cfg_get_param(SECTION_OPTIONS, NULL,
			                         "DefaultInternalEncoding"));
	}

I do not find DefaultInternalEncoding within john.conf??? This being the case, I think I will search for all SECTION_OPTIONS in source (and yes, splitting lines due to the 80 char 'rule' makes this more difficult to check, due to the value being used is frequently on a separate line)

[jfoug]

Ok, I did an audit. I pulled out all SECTION_OPTIONS -> "OptionName". I then wrote a script to find out if the variable is in john.conf.

These 2 are not in the .conf file, BUT we have code that reads the options looking for them.

DefaultInternalEncoding     john.c : 955 and loader.c : 443
Wordfile                    batch.c : 34 and wordlist.c : 648.  NOTE, might be a legacy holdover, and thus valid)

[frank-dittrich]

Commit e2125a7 deprecated the --internal-encoding option and switched to --internal-codepage and also changed the config variable names.

--internal-encoding is only supported for restoring old sessions, so I think --internal-encoding and DefaultInternalEncoding shouln't be advertised.

[frank-dittrich]

A similar change occurred from Wordfile to Wordlist in commit ecb660d.
(Later, prince mode also got its own Wordlist variable.)

[frank-dittrich]

@jfoug
BTW, I first used git log and searched for [Cc]odepage.
Then I used git blame run/john.conf and searched for Word.

[frank-dittrich]

+LogDateFormat =

I am sure the default date logging format isn't an empty string or NULL.
OTOH, if we put the current hard coded fallback here, we only risk getting out of sync if the code changes.

[magnumripper]

OTOH, if we put the current hard coded fallback here

In this case, default is to not use a datestamp at all

[magnumripper]

Commit e2125a7 deprecated the --internal-encoding option and switched to --internal-codepage and also changed the config variable names.

--internal-encoding is only supported for restoring old sessions, so I think --internal-encoding and DefaultInternalEncoding shouln't be advertised.

Exactly! So just disregard them. We'll support the old names in next release, then drop that code

[jfoug]

Exactly! So just disregard them.

Certainly. There were only 2, and both now appear to be deprecated legacy values. All others have been audited, and appear both in source, and in .conf file (with

OTOH, if we put the current hard coded fallback here

In this case, default is to not use a datestamp at all

There are 3 (possibly 4) instances, where the default is 'NULL' only. However, simply making this type change:

--- a/src/logger.c
+++ b/src/logger.c
@@ -299,7 +299,7 @@ static int log_time(void)

        Time = pot.fd >= 0 ? status_get_time() : status_restored_time;

-       if (LogDateFormat) {
+       if (LogDateFormat && *LogDateFormat) {
                struct tm *t_m;
                char Buf[128];
                time_t t;
@@ -613,7 +613,7 @@ void log_event(const char *format, ...)
        if (options.flags & FLG_LOG_STDERR) {
                unsigned int Time;

-               if (LogDateStderrFormat) {
+               if (LogDateStderrFormat && *LogDateStderrFormat) {
                        struct tm *t_m;
                        char Buf[128];
                        time_t t;

which does NOT change the logic behind the default, it simply allows both NULL and "" to be handled exactly the same (as default). I am getting close to a PR, so we can all discuss this once the code has passed the CI's

[magnumripper]

Instead of writing arbitrary things like "tied to #3598" you should use the exact words "closes #3598", mentioned issues will auto close upon merge.