johnmehr / gitup

A minimalist, dependency-free FreeBSD program to clone/pull Git repositories.
BSD 2-Clause "Simplified" License
51 stars 9 forks source link

Don't use <base64.h> #61

Closed michael-o closed 3 years ago

michael-o commented 3 years ago

It just came to my knowledge that if a base system is compiled without Heimdal this header file is not found. Since OpenSSL is a hard requirement from base, consider using EVP_EncodeInit() from OpenSSL to perform Base64 encoding of proxy credentials.

johnmehr commented 3 years ago

I just added a fix for this. How does it look?

michael-o commented 3 years ago

Let me get in touch with my colleague to have this properly tested and get back to you. Thanks!

michael-o commented 3 years ago

Have you forgotten the EVP_ENCODE_CTX_free()?

michael-o commented 3 years ago

It does not compile for me on 12-STABLE:

# make
rm -f .depend
echo gitup.full: /usr/lib/libc.a  >> .depend
Warning: Object directory not changed from original /root/gitup
cc  -O2 -pipe -DCONFIG_FILE_PATH=\"/usr/local/etc/gitup.conf\"   -g -MD  -MF.depend.gitup.o -MTgitup.o -std=gnu99 -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wnested-externs -Wredundant-decls -Wold-style-definition -Wno-pointer-sign -Wmissing-variable-declarations -Wthread-safety -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable  -Qunused-arguments  -c gitup.c -o gitup.o
gitup.c:2339:22: error: variable has incomplete type 'EVP_ENCODE_CTX' (aka 'struct evp_Encode_Ctx_st')
        EVP_ENCODE_CTX      evp_ctx;
                            ^
/usr/include/openssl/ossl_typ.h:100:16: note: forward declaration of 'struct evp_Encode_Ctx_st'
typedef struct evp_Encode_Ctx_st EVP_ENCODE_CTX;
               ^
1 error generated.
*** Error code 1

Stop.
make: stopped in /root/gitup

but does with this patch:

# git diff
diff --git a/gitup.c b/gitup.c
index 1883fed..6a7a7bc 100644
--- a/gitup.c
+++ b/gitup.c
@@ -2336,7 +2336,7 @@ main(int argc, char **argv)
    struct object_node *object = NULL, *next_object = NULL;
    struct file_node   *file   = NULL, *next_file = NULL;
    struct stat         check_file;
-   EVP_ENCODE_CTX      evp_ctx;
+   EVP_ENCODE_CTX      *evp_ctx = EVP_ENCODE_CTX_new();
    const char         *configuration_file = CONFIG_FILE_PATH;
    char               *command = NULL, *start = NULL, *temp = NULL, *extension = NULL, *want = NULL;
    char                base64_credentials[BUFFER_UNIT_SMALL];
@@ -2494,15 +2494,15 @@ main(int argc, char **argv)
            connection.proxy_username,
            connection.proxy_password);

-       EVP_EncodeInit(&evp_ctx);
+       EVP_EncodeInit(evp_ctx);

-       EVP_EncodeUpdate(&evp_ctx,
+       EVP_EncodeUpdate(evp_ctx,
            base64_credentials,
            &base64_credentials_length,
            credentials,
            strlen(credentials));

-       EVP_EncodeFinal(&evp_ctx,
+       EVP_EncodeFinal(evp_ctx,
            base64_credentials,
            &base64_credentials_length);

Does this make sense? I think it is all about local variable vs pointer.

michael-o commented 3 years ago

The corrected version does:

# gitup ports -v2 > out
# Host: git.freebsd.org
# Port: 443
# Proxy Host: de.coia.siemens.net
# Proxy Port: 9400
# Proxy Username: michaelo
# Repository Path: /ports.git
# Target Directory: /usr/ports
# Have: 91f15977cec6282962f2fd6599799b16949e8c21
CONNECT git.freebsd.org:443 HTTP/1.1
Host: git.freebsd.org:443
Proxy-Authorization: Basic bWljaGFlbG86bWljaGFlbG8=

Note: Credentials are bogus and for demo purposes only.

michael-o commented 3 years ago

Also -lroken needs to be removed from the Makefile.

michael-o commented 3 years ago

The colleague has confirmed it to work with the above changes applied.

michael-o commented 3 years ago

Scratch part of my comments. Some functions aren't available with OpenSSL 1.0.2. Based on this information we need ifdefs around this code to support migration from OpenSSL 1.0.2 (11) to 1.1.1 (12+).

johnmehr commented 3 years ago

It doesn't look like EVP_ENCODE_CTX_free() or an equivalent function exists in 1.0.2 (there's no mention of anything like it in the EVP_EncodeInit or evp man pages and I'm not seeing anything in the header files). :(

I added the #ifdefs to handle the differences between 1.0 and 1.1. How does it look on your end?

michael-o commented 3 years ago

The fix is incomplete. The following works for me on 12-STABLE and in a 11.4-RELEASE jail:

diff --git a/Makefile b/Makefile
index f10d552..b56b6a5 100644
--- a/Makefile
+++ b/Makefile
@@ -15,7 +15,7 @@ CONFIG_FILE_PATH=  /usr/local/etc/gitup.conf

 CFLAGS+=    -DCONFIG_FILE_PATH=\"${CONFIG_FILE_PATH}\"

-LDADD= -lssl -lz -lcrypto -lprivateucl -lroken
+LDADD= -lssl -lz -lcrypto -lprivateucl

 WARNS= 6

diff --git a/gitup.c b/gitup.c
index e753cb6..e930e92 100644
--- a/gitup.c
+++ b/gitup.c
@@ -2513,7 +2513,7 @@ main(int argc, char **argv)
             base64_credentials,
             &base64_credentials_length);
 #else
-        evp_ctx = EVP_ENCODE_CTX_New();
+        evp_ctx = EVP_ENCODE_CTX_new();

         EVP_EncodeInit(evp_ctx);

@@ -2527,7 +2527,7 @@ main(int argc, char **argv)
             base64_credentials,
             &base64_credentials_length);

-        EVP_ENCODE_CTX_Free(evp_ctx);
+        EVP_ENCODE_CTX_free(evp_ctx);
 #endif

         /* Remove the trailing return. */
johnmehr commented 3 years ago

Gah! Sorry about that. How does it compile now?

michael-o commented 3 years ago

Works now on both!