add option to load the private_key_password from a file #4938
Conversation
src/main/tls.c
Outdated
| SSL_CTX_set_default_passwd_cb_userdata(ctx, password); | ||
| SSL_CTX_set_default_passwd_cb(ctx, cbtls_password); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Random formatting is not recommended. There's a consistent set of formatting in the server, for good reason.
There was a problem hiding this comment.
You are perfectly right, I tried to follow the existing style but it seems I was not doing a good job, sorry :-)
I just tried to improve it a bit.
I also tried to read https://wiki.freeradius.org/contributing/coding-standards#function-naming-conventions.
If it is still not good, could you provide me where I could find the information about the formatting ?
Is there any formatting tool you might use ?
There was a problem hiding this comment.
See how all of the if and else are formatted in the rest of the code. i.e. } else { is much better than adding tons of whitespace.
There was a problem hiding this comment.
Sorry, must have overlooked this one, I just improved on it.
Hope the formatting is now ok ?
| else { | ||
| ERROR(LOG_PREFIX ": Error reading private_key_password_file %s", conf->private_key_password_file); | ||
| } | ||
| fclose(passwordfile); |
There was a problem hiding this comment.
This will call fclose() when passwordfile is NULL. That is not recommended.
There was a problem hiding this comment.
Are you sure ? There is a check if(passwordfile) and only if it is true (e.g. passwordfile is not NULL) fclose is executed.
Or what I have overlooked ?
There was a problem hiding this comment.
I guess. The random indentation and weird formatting is confusing.
|
It's not clear why this is useful? There isn't a lot of difference between the password being in a FreeRADIUS configuration file, or being in a different file on disk. What's the use-case here? Is this security? Or simplicity, or...?? i.e. I can see that the patch does something, but it would be good to have an explanation as to why the patch is needed. |
|
I use freeradius together with stepca certificate authority. Since stepca supports password stored in files, and we already use stepca that way, this patch would remove the need to have the password stored in multiple files. For me this is more the practical point of view, not so much security. |
No description provided.