Auth response is calculated wrong

Hello davesteinberg,

On the contrary applying your patch causes Authentication to fail in all cases (Varnish 3.0 druapl 6) in my lab. However you are correct the Protocol doesn't call for a newline there yet we are sending one.

Question: Are you loading the secret file with get_file_contents (or similar) as as in case: #1466400: Authentication failure when setting 'varnish_control_key' in settings.php ?

If so I may have been mistaken in my comment to that case and that Varnish does not actually load a newline in at the end of a secret file (Most secret files will have a newline on them) and I will need to read the Varnish code.

In either case I think I need to pull the varnish daemon code up because the sample cli_auth.c file agrees with you on this as well that a newline does not belong.

https://www.varnish-cache.org/trac/browser/lib/libvarnish/cli_auth.c?rev...

Well, found the code quicker then I thought.

From Varnish bin/varnishd/mgt_cli.c

      if (secret_file == NULL) {
                VCLI_Out(cli, "Secret file not configured\n");
                VCLI_SetResult(cli, CLIS_CANT);
                return;
        }
        fd = open(secret_file, O_RDONLY);
        if (fd < 0) {
                VCLI_Out(cli, "Cannot open secret file (%s)\n",
                    strerror(errno));
                VCLI_SetResult(cli, CLIS_CANT);
                return;
        }
        mgt_got_fd(fd);
        VCLI_AuthResponse(fd, cli->challenge, buf);

and from lib/libvarnish/cli_auth.c

void
VCLI_AuthResponse(int S_fd, const char *challenge,
    char response[CLI_AUTH_RESPONSE_LEN + 1])
{
        SHA256_CTX ctx;
        uint8_t buf[BUFSIZ];
        int i;

        assert(CLI_AUTH_RESPONSE_LEN == (SHA256_LEN * 2));

        SHA256_Init(&ctx);
        SHA256_Update(&ctx, challenge, 32);
        SHA256_Update(&ctx, "\n", 1);
        do {
                i = read(S_fd, buf, sizeof buf);
                if (i > 0)
                        SHA256_Update(&ctx, buf, i);
        } while (i > 0);
        SHA256_Update(&ctx, challenge, 32);
        SHA256_Update(&ctx, "\n", 1);
        SHA256_Final(buf, &ctx);
        for(i = 0; i < SHA256_LEN; i++)
                sprintf(response + 2 * i, "%02x", buf[i]);
}

The extra newline in our code is out of place. HOWEVER here is the catch if you enter the string into your browser it will not have the newline that the file has. Of course if you edit the file with vi binary mode with no extra line feeds etc you can create a file that doesn't have a ending newline.

The stock key file generated by the installers (at least on my Ubuntu system I can't recall if I did it from src or a package) has a newline on it.

So how do we handle this is the question, I assume you did a from method or that your key doesn't have a newline on it.

We can't just append a newline onto those entered through the gui (instead of setting it at hash time) because the source auth file could actually be written without a newline (rare but possibly)

My only thoughts are is that we have to write a patch that adds a second checkbox "Secret key file does not have newline" (Because most secret files WILL have the newline) so that when being fed in via the http side we can choose if we append one or not to it when storing in the database. f We would then remove the newline from the generating portion (per the patch above.) This way if a binary match key file is provided from the config file it will work and if entered via the GUI we can choose to append the newline pragmatically or not.

Just an FYI that the solution for adding $conf['varnish_control_key'] in settings.php posted here: http://drupal.org/node/1466400#comment-5872368 in #1466400: Authentication failure when setting 'varnish_control_key' in settings.php would change if this behavior is changed.

Attached find 2 patches.

Version 6x and 7x branches.

NOTE: This is only tested on the 6x branch as I only have a 6x lab. 7x branch is same patch but different character spacing required a new patch.

This is upgrade compatible and will not affect #1466400 until a user makes a conscious change.

Adds a checkbox below control key with a default of selected to append a new line.

Splits the authentication send based on this to either append it by us (as is currently done) or to not append it (if you use an fopen to get the key for example)

30 Day bump.

Anyone trying this so it can be tagged tested for future commit?

Hi cmlara, I've applied the varnish-v6-AuthCalcFix-1492670-2.patch to Varnish-6.x-1.1 running under Pressflow. It applied successfully, but I'm still not able to connect to the Varnish control terminal running on another VM. This was working 2 days ago, prior to a site rebuild (i.e. updating all contrib modules), and I can get it to work when I disable the "-S" secret flag to the varnish daemon. I've tried with and without the newline character in the secret file and with the checkbox provided by your patch. Any ideas?

Hello Klokie,

Well the obvious question that has to be asked is 'is the secret correct'
If it is then it comes down to varnish_control_key_appendnewline in variable_get() being the next factor.

The built in drupal form submission should be handling it by itself without any need to put in a special form handler (so I didn't) but that would be my next big question is seeing if the variable is getting changed ( select * from variable where name = "varnish_control_key_appendnewline" ; ) and looking at the varnishd log outputs to see what responses are being thrown.

with varnish_control_key_appendnewline being true the system should respond as was the original method, with it being false it should not append a newline.

No longer applies.

Patch re-rolled.

Why aren't the bots engaging???

Would it be simpler to just do a rtrim("\n") on the secret string after it's loaded?

There should also be some documentation explaining that the server's secret file needs a newline at the end of the file (a problem we ran into).

This patch updates the README.txt and admin settings field's description to state that the secret must end in a newline character, and rtrim()'s the secret string to remove any newline.

I don't see #13 being valid since it still forces users to have a newline in their varnish secret file. It is possible a user will not be able to change the secret (shared among other programs, set by a 3rd party, etc)

It is 100% valid for Varnish to have a secret data that does not end in a newline. This could happen if read from /dev/urandom or similar or if the file was just written without a newline.

I don't have any test labs up at this time, I was starting to get active in Drupal Dev a number of years back but like often happens in life I got pulled a dozen different directions and Drupal dropped off my list of highest priorities. #9 looks best to me (of course I am biased as its a reroll of what I originally submitted). I would suggest if you could test #9 and move it forward to push this to get the required code approvals it would be the most flexible solution.

We're using #9 in several of our production sites and would love to see it merged in latest dev. Setting to RTBC but only for patch #9. Please ignore #13

Thanks. Went for patch in #9. Committed to latest dev.