diff mbox series

[ovs-dev,v2,2/4] ovs-pki: Fix file permissions on Windows.

Message ID 20240301211045.3714106-3-i.maximets@ovn.org
State Accepted
Commit f5fa9a0a3cffc9cfa140acda69d5e7c8ac201fee
Delegated to: Ilya Maximets
Headers show
Series Windows: Fix OpenSSL build and ovs-pki. | 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 March 1, 2024, 9:10 p.m. UTC
There is no chmod or 'mkdir -m' support on Windows, so setting file
permissions for keys and certificates doesn't actually work.

Implementing them using icacls utility instead.

ovs-pki script currently only uses 0700 and 0750 modes, so only those
(and 0600) are implemented.  NTFS ACLs on Windows are fairly different
and more complex in comparison with Unix file permissions, so it's hard
to implement these functions in a generic way.  The script will fail if
it will encounter an unknown mode.

0700 is implemented as a F (full access) for 'Creator Owner' with no
other permissions.  0750 has an additional RX (read+execute) for the
'Creator Group'.  0600 is implemented the same as 0700, since it
doesn't matter for this use case to have or not to have an executable
or traversal permissions managed separately from everything else and
it would be a little overly verbose to give all the permissions except
for X.

Inheritance rules are set to (OI)(CI), so the folder itself, subfolders
and files in a folder inherit those ACEs.

'umask' also doesn't work on Windows.  Instead, moving the private key
output files to a temporary folder that has restricted access already
configured.  The file will inherit these restricted ACEs.
It should not be necessary to set explicit permissions for these
files since moving them within the same volume should preserve ACEs.
However, it might be safer to chmod them directly as well, just in
case.  Windows administrators will still have to be careful with
private keys, because file copies do not preserve permissions and
moves to different volumes do not preserve them as well.  'robocopy'
with flags to copy security should be used in these cases.  We may
want to re-implement 'mv' with 'robocopy' if that becomes a problem
in the future.

There is one more place where umask is used in the script for
creation of a self-signed certificate, but it is not actually needed
there since the resulted certificate doen't need to be private, so
not changing this part for now.

Tested with running an empty 'make check' in AppVeyor and examining
permissions for files in tests/pki:

       Files          |   Linux    |             Windows
 ---------------------+------------+--------------------------------------
 controllerca         | drwxr-xr-x | NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
 switchca             |            | BUILTIN\Administrators:(I)(OI)(CI)(F)
 *ca\certs            |            | BUILTIN\Users:(I)(OI)(CI)(RX)
 *ca\crl              |            | BUILTIN\Users:(I)(CI)(AD)
 *ca\newcerts         |            | BUILTIN\Users:(I)(CI)(WD)
                      |            | APPVEYOR-VM\appveyor:(I)(F)
                      |            | CREATOR OWNER:(I)(OI)(CI)(IO)(F)
 ---------------------+------------+--------------------------------------
 stamp                | -rw-r--r-- | NT AUTHORITY\SYSTEM:(I)(F)
 test-cert.pem        |            | BUILTIN\Administrators:(I)(F)
 test-req.pem         |            | BUILTIN\Users:(I)(RX)
 test2-cert.pem       |            | APPVEYOR-VM\appveyor:(I)(F)
 test2-req.pem        |            |
 *ca\ca.cnf           |            |
 *ca\cacert.pem       |            |
 *ca\careq.pem        |            |
 *ca\crlnumber        |            |
 *ca\index.txt*       |            |
 *ca\serial*          |            |
 *ca\newcerts\*.pem   |            |
 ---------------------+------------+--------------------------------------
 controllerca\private | drwx------ | APPVEYOR-VM\appveyor:(F)
 switchca\private     |            | CREATOR OWNER:(OI)(CI)(IO)(F)
 ---------------------+------------+--------------------------------------
 test-privkey.pem     | -rw------- | APPVEYOR-VM\appveyor:(F)
 test2-privkey.pem    |            |
 *ca\private\cakey.pem|            |

We can see that private folders and keys have only a full access from
their owners.  Other files and folders have some extra inherited ACEs
from a containing folder.

Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 utilities/ovs-pki.in | 87 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)
diff mbox series

Patch

diff --git a/utilities/ovs-pki.in b/utilities/ovs-pki.in
index b0c538903..3d2ef911c 100755
--- a/utilities/ovs-pki.in
+++ b/utilities/ovs-pki.in
@@ -57,6 +57,77 @@  FreeBSD|NetBSD|Darwin)
     ;;
 esac
 
+case $(uname -s) in
+MINGW*|MSYS*)
+    chmod()
+    {
+        local PERM=$1
+        local FILE=$2
+        local INH=
+
+        if test -d "${FILE}"; then
+            # Inheritance rules for folders: apply to a folder itself,
+            # subfolders and files within.
+            INH='(OI)(CI)'
+        fi
+
+        case "${PERM}" in
+        *700 | *600)
+            # Reset all own and inherited ACEs and grant full access to the
+            # "Creator Owner".  We're giving full access even for 0600,
+            # because it doesn't matter for a use case of ovs-pki.
+            icacls "${FILE}" /inheritance:r /grant:r "*S-1-3-0:${INH}F"
+            ;;
+        *750)
+            # Reset all own and inherited ACEs, grant full access to the
+            # "Creator Owner" and a read+execute access to the "Creator Group".
+            icacls "${FILE}" /inheritance:r /grant:r \
+                "*S-1-3-0:${INH}F" "*S-1-3-1:${INH}RX"
+            ;;
+        *)
+            echo >&2 "Unable to set ${PERM} mode for ${FILE}."
+            exit 1
+            ;;
+        esac
+    }
+
+    mkdir()
+    {
+        ARG_P=
+        PERM=
+        for arg; do
+            shift
+            case ${arg} in
+            -m?*)
+                PERM=${arg#??}
+                continue
+                ;;
+            -m)
+                PERM=$1
+                shift
+                continue
+                ;;
+            -p)
+                ARG_P=-p
+                continue
+                ;;
+            *)
+                set -- "$@" "${arg}"
+                ;;
+            esac
+        done
+
+        command mkdir ${ARG_P} $@
+        if [ ${PERM} ]; then
+            for dir; do
+                shift
+                chmod ${PERM} ${dir}
+            done
+        fi
+    }
+    ;;
+esac
+
 for option; do
     # This option-parsing mechanism borrowed from a Autoconf-generated
     # configure script under the following license:
@@ -466,14 +537,24 @@  CN = $cn
 [ v3_req ]
 subjectAltName = DNS:$cn
 EOF
+    # It is important to create private keys in $TMP because umask doesn't
+    # work on Windows and permissions there are inherited from the folder.
+    # umask itself is still needed though to ensure correct permissions
+    # on non-Windows platforms.
     if test $keytype = rsa; then
-        (umask 077 && openssl genrsa -out "$1-privkey.pem" $bits) 1>&3 2>&3 \
-            || exit $?
+        (umask 077 && openssl genrsa -out "$TMP/privkey.pem" $bits) \
+            1>&3 2>&3 || exit $?
     else
         must_exist "$dsaparam"
-        (umask 077 && openssl gendsa -out "$1-privkey.pem" "$dsaparam") \
+        (umask 077 && openssl gendsa -out "$TMP/privkey.pem" "$dsaparam") \
             1>&3 2>&3 || exit $?
     fi
+    # Windows: applying permissions (ACEs) to the file itself, just in case.
+    # 'mv' should technically preserve all the inherited ACEs from a TMP
+    # folder, but it's better to not rely on that.
+    chmod 0600 "$TMP/privkey.pem"
+    mv "$TMP/privkey.pem" "$1-privkey.pem"
+
     openssl req -config "$TMP/req.cnf" -new -text \
         -key "$1-privkey.pem" -out "$1-req.pem" 1>&3 2>&3
 }