Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt. --subca-len: basicConstraints CA extension, Append 'pathlen:N' #706

Merged
merged 5 commits into from Sep 28, 2022

Conversation

TinCanTech
Copy link
Collaborator

@TinCanTech TinCanTech commented Sep 24, 2022

When signing a request for an intermediate CA using --subca-len=N:

For a Sub-CA, the current method to apply 'pathlen:N' to CA basicConstraints over-writes all user set basicConstraints.

Replace that with an awk script which reads the current x509-types/ca file; selects the last occurence of 'basicConstraints' (As does OpenSSL) and then prints that line, with ", pathlen:$EASYRSA_SUBCA_LEN" appended, into the temporary x509-types/ca file.

If no CA basicConstraint is found then exit with an error. Reason:

Easy-RSA default CA basicConstrain will always be defined. If that is changed by the user, who then attempts to use Easy-RSA to append 'pathlen' then that is an error. Easy-RSA must not insert a default when the default has been deliberately removed.

Closes: #691 - Original bug report.
Closes: #692 - First use of awk as a solution. [Credit]

Additional: Move the confirmation dialogue to after all the configuration has
been completed.

Signed-off-by: Richard T Bonhomme tincantech@protonmail.com

When signing a request for an intermediate CA using --subca-len=N:

For a Sub-CA, the current method to apply 'pathlen:N' to CA basicConstraints
over-writes all user set basicConstraints.

Replace that with an awk script which reads the current x509-types/ca file;
selects the last occurence of 'basicConstraints' (As does OpenSSL) and then
prints that line, with ", pathlen:$EASYRSA_SUBCA_LEN" appended, into the
temporary x509-types/ca file.

If no CA basicConstraint is found then exit with an error. Reason:

Easy-RSA default CA basicConstrain will always be defined. If that is changed
by the user, who then attempts to use Easy-RSA to append 'pathlen' then that
is an error. Easy-RSA must not insert a default when the default has been
deliberately removed.

Closes: OpenVPN#691 - Original bug report.
Closes: OpenVPN#692 - First use of awk as a solution. [Credit]

Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>
Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>
@TinCanTech
Copy link
Collaborator Author

If successfully reviewed, this could be bumped up to v3.1.1.

if [ "$crt_type" = "ca" ] && [ "$EASYRSA_SUBCA_LEN" ]; then
# Print the last occurence of basicContraints in x509-types/ca
# If basicContraints not defined then bail
awkscript='/^[[:blank:]]*basicConstraints[[:blank:]]*=/ { bC=$0 }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this missing a # shellcheck disable=SC2016 # vars don't ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and it is also guilty of SC2094 (info): Make sure not to read and write the same file in the same pipeline.. Update on the way.

@dekeonus thanks!

Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>
@TinCanTech
Copy link
Collaborator Author

TinCanTech commented Sep 25, 2022

One final patch is required to correct the error output. On the way..

Edit: And update ChangeLog

Move show_host() to cleanup() and only call it when die() was called.
This allows for confirm() Aborted to exit without extended error data.

Move detect_host after options processing. Allows for use of options.
eg: --verbose

Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>
Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>
@TinCanTech TinCanTech added Full-Approval Merge is imminent and removed initial-approval development Possible changes labels Sep 26, 2022
@TinCanTech TinCanTech added the Community reveiwed Genuine community participation label Sep 26, 2022
Copy link

@dekeonus dekeonus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

die() still has output to stdout, and this causes the || die "basicConstraints is no defined .." to still dump show_host() and now information() "Temp session preserved.. " into the temporary extension file.

The operation fails as expected, but those ostensibly error / logging messages aren't being displayed to the user.

basicConstraints="$(
awk "$awkscript" "$EASYRSA_EXT_DIR/$crt_type"
)" || die "\
basicConstraints is not defined, cannot use 'pathlen'"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still seeing show_host() and cleanup() information in $ext_tmp :(
easyrsa --keep-tmp=foo1 --subca-len=99 sign-req ca ssubtCA

# === Trimmed ===
#basicConstraints = CA:TRUE                                                                                             
#basicConstraints = CA:TRUE, pathlen:9                                                                                  
subjectKeyIdentifier = hash                                                                                             
authorityKeyIdentifier = keyid:always,issuer:always                                                                     
keyUsage = cRLSign, keyCertSign                                                                                         
* Temp session preserved: /home/dekeonus/devel/easyrsa/pki_root/subtCA/tmp/foo1                                         
                                                                                                                        
                                                                                                                        
EasyRSA Version Information                                                                                             
Version:     git-706r2                                                                                                  
Generated:   2022-09-27T20:22:05 AEST                                                                                   
SSL Lib:     OpenSSL 1.1.1q  5 Jul 2022                                                                                 
Git Commit:  43e5cb9af1decd6a09734b3955fea37b4be7d999                                                                   
Source Repo: https://github.com/OpenVPN/easy-rsa                                                                        
Host: git-706r2 | nix | Linux | /bin/bash | OpenSSL 1.1.1q  5 Jul 2022 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed by 96b3d38

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still seeing the issue:

# === trimmed ===
#basicConstraints = CA:TRUE, pathlen:9
subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid:always,issuer:always
keyUsage = cRLSign, keyCertSign
* Temp session preserved: /home/dekeonus/devel/easyrsa/pki_root/subtCA/tmp/foo3


EasyRSA Version Information
Version:     git-706-r3
Generated:   2022-09-27T21:02:38 AEST
SSL Lib:     OpenSSL 1.1.1q  5 Jul 2022
Git Commit:  96b3d3884722994766fa9f557886be84f265f431
Source Repo: https://github.com/OpenVPN/easy-rsa
Host: git-706-r3 | nix | Linux | /bin/bash | OpenSSL 1.1.1q  5 Jul 2022

Copy link

@dekeonus dekeonus Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shasum easyrsa for the unversioned easyrsa script I have here (i.e. ~VER~ ~DATE~ et.al.)
421c9ceb7d8a68f023914db35f3a423a3f713869 easyrsa

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a minor merge conflict but after successful merge you should have this:

die() {
	print "
Easy-RSA error:

$1
" 1>&2
	die_error_exit=1
	exit "${2:-1}"
} # => die()

die() does not call show_host().

Then

cleanup() {
	if [ "${EASYRSA_TEMP_DIR_session%/*}" ] && \
		[ -d "$EASYRSA_TEMP_DIR_session" ]
	then
		# Remove temp-session or create temp-snapshot
		if [ "$EASYRSA_KEEP_TEMP" ]
		then
			# skip on black-listed directory names, with a warning
			if [ -d "$EASYRSA_TEMP_DIR/$EASYRSA_KEEP_TEMP" ]
			then
				warn "\
Prohibited value for --keep-tmp: '$EASYRSA_KEEP_TEMP'
Temporary session not preserved."
			else
				# create temp-snapshot
				keep_tmp="$EASYRSA_TEMP_DIR/tmp/$EASYRSA_KEEP_TEMP"
				mkdir -p "$keep_tmp"
				rm -rf "$keep_tmp"
				mv -f "$EASYRSA_TEMP_DIR_session" "$keep_tmp"
				print "Temp session preserved: $keep_tmp"
			fi
		fi

		# Always remove temp-session
		rm -rf "$EASYRSA_TEMP_DIR_session"
	fi

	if [ "${EASYRSA_EC_DIR%/*}" ] && [ -d "$EASYRSA_EC_DIR" ]
	then
		rm -rf "$EASYRSA_EC_DIR"
	fi

	# Remove files when build_full()->sign_req() is interrupted
	[ "$on_error_build_full_cleanup" ] && \
		rm -f "$crt_out" "$req_out" "$key_out"

	# Restore files when renew is interrupted
	[ "$on_error_undo_renew_move" ] && renew_restore_move; :
	# Restore files when rebuild is interrupted
	[ "$on_error_undo_rebuild_move" ] && rebuild_restore_move; :

	# shellcheck disable=SC3040 # In POSIX sh, set option [name] is undefined
	case "$easyrsa_host_os" in
		nix) [ -t 1 ] && stty echo ;;
		win)
			if [ "$easyrsa_win_git_bash" ]; then
				[ -t 1 ] && stty echo
			else
				set -o echo
			fi
		;;
		*) warn "Host OS undefined."
	esac

	if [ "$1" = ok ] || [ "$EASYRSA_BATCH" ] || \
		[ "$EASYRSA_SILENT" ] || [ "$EASYRSA_QUIET" ]
	then
		: # ok
	else
		print # just to get a clean line
	fi

	# Exit with error 1, if an error ocured...
	if [ "$easyrsa_error_exit" ]; then
		# Set by verify_cert() for full error-out
		exit 1
	elif [ "$1" = ok ]; then
		# if there is no error then 'cleanup ok' is called
		exit 0
	else
		# if 'cleanup' is called without 'ok' then an error occurred
		# Do not show_host() for confirm() aborted exit
		[ "$die_error_exit" ] && show_host
		exit 1
	fi
} # => cleanup()

cleanup() has the new safe version of --keep-tmp and only calls show_host() on an error sent to die(). This allows for aborted confirm() to exit without extended error data.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the code I've got here, and what I was running.

I opened the commit link 96b3d38 , selected browse files, used the download code link and copied the easyrsa script to my test location. and then modified the version strings by hand¹ (so that I have a reference for what code I'm running in the test terminal).

I've got modifications in my working dir, and I didn't want to git stash them.

¹ in my normal workflow I use a script to call the build-dist.sh with the local branch name as version for testing.

Copy link

@dekeonus dekeonus Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that cleanup() function you listed does not match 96b3d38

but the final stanzas around # Exit with error 1, if an error ocured... do match

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have applied patches out of order.

Copy link

@dekeonus dekeonus Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was downloading the file (entire not patches) from github.
I've now, in a fresh never before used directory, performed:
git clone https://github.com/TinCanTech/easy-rsa.git
git fetch origin fix-subca-len
git switch fix-subca-len
git log -1

commit 43e5cb9af1decd6a09734b3955fea37b4be7d999 (HEAD -> fix-subca-len, origin/fix-subca-len)
Author: Richard T Bonhomme <>
Date:   Sun Sep 25 21:42:26 2022 +0100

    ChangeLog: Add resolution of --subca-len=N issue

    Signed-off-by: Richard T Bonhomme <>

deploying easyrsa into my test environment with that TinCanTech/fix-subca-len branch still results in show_host() and information() ending in the temp extension file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to apply this to 318e57b

@TinCanTech TinCanTech merged commit f4cb94a into OpenVPN:master Sep 28, 2022
@TinCanTech TinCanTech deleted the fix-subca-len branch October 28, 2022 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sign-req ca does not honour user set critical flag in "ca" type if --subca-len provided
2 participants