sec: Adjust flag validation for TLS.

[royjacobson]

This changes our authentication policy when using TLS:

1. If TLS is enabled, some authentication must be used.
2. Password authentication is allowed for client authentication.

[kostasrim]

also, plz expand replication_test.py with the new flag combinations

[kostasrim]

So for some reason, I could not mark the whole code block and I could not suggest the changes in one comment.

Basically, in summary, for the Validate functions you can:

1. Mark them const since they don't modify any data member
2. Remove the boolean flag and return. When you set the bool flag to true the rest of the checks are redundant

[royjacobson]

So for some reason, I could not mark the whole code block and I could not suggest the changes in one comment.

Basically, in summary, for the Validate functions you can:

1. Mark them `const` since they don't modify any data member

2. Remove the boolean flag and return. When you set the bool flag to `true` the rest of the checks are redundant

I disagree about the has_auth changes - I wrote this code in this way so it's easy to understand exactly what properties the function guarantees and to validate it works correctly. I feel that early returns make it a bit less clear (even though it's longer).

About your first point, those are free functions :)

[kostasrim]

I disagree about the has_auth changes - I wrote this code in this way so it's easy to understand exactly what properties the function guarantees and to validate it works correctly. I feel that early returns make it a bit less clear (even though it's longer).

I think the opposite is true and adding this flag adds additional cognitive overhead which is not really needed. It adds unnecessary logic to the function. For example:

void ValidateClientTlsFlags() {
  if (!absl::GetFlag(FLAGS_tls_replication)) {   //1
    return;
  }
  
  bool has_auth = false;
  
  if (!absl::GetFlag(FLAGS_tls_key_file).empty()) {
    if (absl::GetFlag(FLAGS_tls_cert_file).empty()) {
       LOG(ERROR) << "tls_cert_file flag should be set";
       exit(1);
    }
    has_auth = true;                                                                //2
 }
 
 if (!absl::GetFlag(FLAGS_masterauth).empty())
   has_auth = true;                                                                //3
}

1. First and foremost, why would I care what happens after the function if line //2 is true? Like what value does it bring for me to go over the whole function? I have all the information I need at the right spot.
2. Let's say something happens and now I need to debug this and figure out when has_auth became true. I can put a break point to the return at the certain line and I know what happens if I don't hit it -- that is I returned earlier than the break point. Yes I can do the same with watchpoints but that's too much. Whereas with what you have now, I need to step through and follow the logic. Imagine doing this for a large call stack, yikes!

p.s. Early returns are widely accepted as logic simplification and is something a lot of the committee members like Sean Parent, Chris Di Bella really endorse and I have personally seen in their code ;)

p.s. 2 Clang-tidy has a nice check about this called readability-else-after-return

What you are doing here, is similar to this anti-pattern only at larger scope with a boolean variable instead of early returning

Feel free to ignore me, but this IMO is not more readable. Just let me know what you decide -- I will be ok with both!

[royjacobson]

@kostasrim 1 - some combinations of flags (like 'key' flag without a 'cert' flag) result in exception instead of an early return. And 2, I agree about early return in general but I feel this is a better way to write security critical code like auth policies 😅. So I'd like to stick with this version