diff mbox series

[ovs-dev] ovs-pki: Remove umask trick for self-signing.

Message ID 20240213194442.1590625-1-i.maximets@ovn.org
State Accepted
Commit c7dd0a7b09add427555a986d504b012240f49a28
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] ovs-pki: Remove umask trick for self-signing. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets Feb. 13, 2024, 7:44 p.m. UTC
The output file of this openssl command is a certificate signed with
pre-existing private key.  It doesn't create a private key.   The
restricted permissions are explicitly removed from the resulted
certificate right after its generation.  So, there is no point in
creating it with restricted permissions in the first place.

Fixes: 99e5e05db37a ("ovs-pki: Create private keys with restricted permissions.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 utilities/ovs-pki.in | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Comments

Mike Pattrick Feb. 13, 2024, 8:46 p.m. UTC | #1
On Tue, Feb 13, 2024 at 2:44 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> The output file of this openssl command is a certificate signed with
> pre-existing private key.  It doesn't create a private key.   The
> restricted permissions are explicitly removed from the resulted
> certificate right after its generation.  So, there is no point in
> creating it with restricted permissions in the first place.
>
> Fixes: 99e5e05db37a ("ovs-pki: Create private keys with restricted permissions.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Acked-by: Mike Pattrick <mkp@redhat.com>
Simon Horman Feb. 15, 2024, 1:18 p.m. UTC | #2
On Tue, Feb 13, 2024 at 08:44:41PM +0100, Ilya Maximets wrote:
> The output file of this openssl command is a certificate signed with
> pre-existing private key.  It doesn't create a private key.   The
> restricted permissions are explicitly removed from the resulted
> certificate right after its generation.  So, there is no point in
> creating it with restricted permissions in the first place.
> 
> Fixes: 99e5e05db37a ("ovs-pki: Create private keys with restricted permissions.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Hi Ilya,

I'm fine with this change, and agree with the explanation provided.
However, it is not clear to me that this is a fix, for which
my working definition is a user-visible problem, usually at run-time.

That notwithstanding, feel free to add.

Acked-by: Simon Horman <horms@ovn.org>

...
Ilya Maximets Feb. 15, 2024, 10:53 p.m. UTC | #3
On 2/15/24 14:18, Simon Horman wrote:
> On Tue, Feb 13, 2024 at 08:44:41PM +0100, Ilya Maximets wrote:
>> The output file of this openssl command is a certificate signed with
>> pre-existing private key.  It doesn't create a private key.   The
>> restricted permissions are explicitly removed from the resulted
>> certificate right after its generation.  So, there is no point in
>> creating it with restricted permissions in the first place.
>>
>> Fixes: 99e5e05db37a ("ovs-pki: Create private keys with restricted permissions.")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Hi Ilya,
> 
> I'm fine with this change, and agree with the explanation provided.
> However, it is not clear to me that this is a fix, for which
> my working definition is a user-visible problem, usually at run-time.

Makes sense.  The Fixes tag here is more for the demonstration of the
original intent of the code, i.e. highlighting that it doesn't do what
it was meant to do.  We could backport it, but I agree that it is not
needed, so I won't.

> 
> That notwithstanding, feel free to add.
> 
> Acked-by: Simon Horman <horms@ovn.org>

Thanks, Mike and Simon!  Applied.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/utilities/ovs-pki.in b/utilities/ovs-pki.in
index e0ba910f9..d20f6eb98 100755
--- a/utilities/ovs-pki.in
+++ b/utilities/ovs-pki.in
@@ -545,16 +545,9 @@  elif test "$command" = self-sign; then
     cat > "$TMP/v3.ext" <<EOF
 subjectAltName = DNS:$arg1
 EOF
-
-    # Create both the private key and certificate with restricted permissions.
-    (umask 077 && \
-     openssl x509 -in "$arg1-req.pem" -out "$arg1-cert.pem.tmp" \
-                  -signkey "$arg1-privkey.pem" -req -days 3650 -text \
-                  -extfile $TMP/v3.ext) 2>&3 || exit $?
-
-    # Reset the permissions on the certificate to the user's default.
-    cat "$arg1-cert.pem.tmp" > "$arg1-cert.pem"
-    rm -f "$arg1-cert.pem.tmp"
+    openssl x509 -in "$arg1-req.pem" -out "$arg1-cert.pem" \
+                 -signkey "$arg1-privkey.pem" -req -days 3650 -text \
+                 -extfile $TMP/v3.ext 2>&3 || exit $?
 else
     echo "$0: $command command unknown; use --help for help" >&2
     exit 1