CFE-2613: Include and package cf-keycrypt with core #2883

olehermanse · 2017-07-06T01:00:19Z

WIP, do not merge yet.

TODO:

Edit enterprise and buildscripts repos for new binaries.
Fix compiler warnings.
@jimis: Please review cf-keycrypt.c which is from the cf-keycrypt repository
@nickanderson: Please add an acceptance test in your own commit (you should be able to push to this branch)
@nickanderson writes documentation (CFE-2613: Add documentation for cf-keycrypt documentation#1622)
@nickanderson add example in core
Manually test that the built binary works for common use cases
Reimplement crypto code to be secure for large files

olehermanse · 2017-07-06T08:22:43Z

For future reference, these were the compiler warnings of the unmodified cf-keycrypt source:

Making all in cf-keycrypt
  CC     cf-keycrypt.lo
cf-keycrypt.c: In function ‘get_host_pubkey’:
cf-keycrypt.c:99:9: error: unused variable ‘error’ [-Werror=unused-variable]
cf-keycrypt.c:94:9: error: unused variable ‘ecode’ [-Werror=unused-variable]
cf-keycrypt.c:88:10: error: unused variable ‘value’ [-Werror=unused-variable]
cf-keycrypt.c:87:11: error: unused variable ‘key’ [-Werror=unused-variable]
cf-keycrypt.c: In function ‘rsa_encrypt’:
cf-keycrypt.c:166:27: error: variable ‘len’ set but not used [-Werror=unused-but-set-variable]
cc1: all warnings being treated as errors
make[2]: *** [cf-keycrypt.lo] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

jimis

First review, I only made it through a small section so more to come.

Also remove the commented out sections like // unused variable etc.

Thanks for starting this @olehermanse !

jimis · 2017-07-06T16:13:16Z

cf-keycrypt/cf-keycrypt.c

+#include <known_dirs.h>
+#include <dbm_api.h>
+/*
+#ifdef LMDB


This comment can probably be removed.

jimis · 2017-07-06T16:15:32Z

cf-keycrypt/cf-keycrypt.c

+*/
+
+#include <config.h>
+#include <cf3.defs.h>


Always first inclusion must be

either platform.h, which itself includes config.h.

OR cf3.defs.h which includes itself platform.h and a bunch of libpromises stuff.

I think the 2nd is what you want here.

jimis · 2017-07-06T16:16:25Z

cf-keycrypt/cf-keycrypt.c

+*/
+
+#define STDIN     0
+#define STDOUT    1


Unused defines. Remove?

jimis · 2017-07-06T16:23:19Z

cf-keycrypt/cf-keycrypt.c

+    printf("Workdir is defined as %s\n", WORKDIR);
+    printf(
+    "\n"
+    "Usage: cf-keycrypt [-e public-key|-d private-key|-H hostname-or-ip] -o outfile -i infile [-h]\n"


I've never used this tool, but the command line usage that I would expect based on other tools is:

cf-keycrypt -e filename > file.out cf-keycrypt -o file.out -e filename

Those two are equivalent. Similar for -d. Key is found in WORKDIR unless an option like --key=/path/to/key is specified.

Apparently the command needs more options than my previous examples. It needs a key file OR a hostname/IP.

-e or --encrypt

-d or --decrypt

-k or --keyfile

-H or --host

-o or --output

filename should be a mandatory argument

It's important to agree to a sensible command line usage before merging so that we don't break things later.

I think @nickanderson is in favor of getting this merged fairly quickly, and in the long term adding it's functionality to cf-key and deprecating this tool before the next LTS.

I can tell you one thing: after it gets merged, nobody will have time to actually do anything. And we have time-based releases, not feature-based, so the release will happen.

So what you merge is what you'll get, almost certainly.

Let's clean up the cli usage. I can always argue and beg for backporting, thats easier than allocating time to merge functionality into cf-key when we already have something that works and is standalone.

jimis · 2017-07-06T16:28:29Z

cf-keycrypt/cf-keycrypt.c

+
+    for(res = result; res != NULL && !found; res = res->ai_next)
+    {
+        inet_ntop(res->ai_family, get_in_addr((struct sockaddr *)res->ai_addr), ipaddress, sizeof(ipaddress));


Remove get_in_addr() and inet_ntop(). The modern way to convert a struct sockaddr to textual representation of address is getnameinfo(). Example:

{ /* Convert IP address to string, no DNS lookup performed. */ char txtaddr[CF_MAX_IP_LEN] = ""; getnameinfo(ap->ai_addr, ap->ai_addrlen, txtaddr, sizeof(txtaddr), NULL, 0, NI_NUMERICHOST); Log(LOG_LEVEL_DEBUG, "Bound to address '%s' on '%s' = %d", txtaddr, CLASSTEXT[VSYSTEMHARDCLASS], VSYSTEMHARDCLASS); }

Now that I think about all this whole address lookup function get_host_pubkey(), it has many issues in the lookups, and we have Hostname2IPString() for that. So better use that and save some code and risk.

@jimis This is not a straight forward replace. Consider the steps here:

getaddrinfo() returns a list of results for the given hostname

Loop through the list. If 127.0.0.1 or ::1 is in that list and before Address2Hostkey finds a hostname, return ppkeys/localhost.pub

If Address2Hostkey succeeded during 2., return ppkeys/root-<KEY>.pub

If not loop through list again. Return any ppkeys/root-<IP-ADDRESS>.pub where that file exists.

This is what I understand. I'm sure it's flawed, but what is the desired behavior here?

jimis · 2017-07-06T16:35:06Z

cf-keycrypt/cf-keycrypt.c

+    struct addrinfo *res;
+    bool found = false;
+
+    int error = getaddrinfo(host, NULL, NULL, &result);


I see getaddrinfo() but no freeaddrinfo(); leaking memory?

jimis · 2017-07-06T16:44:42Z

cf-keycrypt/cf-keycrypt.c

+/*
+   cf-keycrypt.c
+
+   Copyright (C) 2017 cfengineers.net


I think we should add our copyright notice as well. Maybe also convert the language to GPLv3. Pinging @kacf and @nickanderson.

We should definitely mention the primary author of this in the CONTRIBUTORS file, and probably ping him in this pull request, what's his handle?

A file from external contributors should have the license intact, unless we've made substantial changes. Apache can be relicensed under GPLv3, so this is no problem for the project as a whole

@kacf given that we are making significant changes to it, I suggest keeping the initial license intact, but prepend our license and copyright for 2017. Sounds right?

Yep, that's fine!

Prepended our license.

jimis

I still have to review the actual crypto code.

jimis · 2017-07-06T20:15:39Z

cf-keycrypt/cf-keycrypt.c

+        for(res = result; res != NULL; res = res->ai_next)
+        {
+            inet_ntop(res->ai_family, get_in_addr((struct sockaddr *)res->ai_addr), ipaddress, sizeof(ipaddress));
+            snprintf(buffer, BUFSIZE * sizeof(char), "%s/ppkeys/root-%s.pub", WORKDIR, ipaddress);


This whole thing can most probably be thrown out. I think we can just use crypto.c:HavePublicKey().

jimis · 2017-07-06T21:06:28Z

cf-keycrypt/cf-keycrypt.c

+{
+    FILE *fp = NULL;
+    RSA* PRIVKEY = NULL;
+    static char *passphrase = "Cfengine passphrase";


This exists all over our codebase. Move all separate definitions to cf3.defs.h. Consolidate this readseckey() function together with the existing code in crypto.c, export the one from there and import it here.

I would rather not touch crypto or defsin this PR/commit. I can do that as a separate step after, if you'd like.

jimis · 2017-07-06T21:06:52Z

cf-keycrypt/cf-keycrypt.c

+{
+    FILE *fp;
+    RSA *key = NULL;
+    static char *passphrase = "Cfengine passphrase";


same as above for this phrase and for readpubkey function.

jimis · 2017-07-06T21:08:05Z

cf-keycrypt/cf-keycrypt.c

+int main(int argc, char *argv[])
+{
+    OpenSSL_add_all_algorithms();
+    ERR_load_crypto_strings();


CryptoInitialize()

jimis · 2017-07-06T21:16:00Z

Plenty of code style issues: braces not on separate lines, implicit comparisons, unneeded casts, uneeded parentheses etc.

jimis · 2017-07-06T21:23:52Z

cf-keycrypt/cf-keycrypt.c

+                return -1;
+            }
+        }
+        fwrite(tmpplain,1,strlen(tmpplain),outfile);


This must be checked for error return.

Fixed, and a bunch of other fread and fwrite.

jimis · 2017-07-06T21:25:47Z

cf-keycrypt/cf-keycrypt.c

+
+    ks = RSA_size(key);
+    tmpciph = (unsigned char *)malloc(ks * sizeof(unsigned char));
+    tmpplain = (unsigned char *)malloc(ks * sizeof(unsigned char));


remove casts and sizeofs.

Also use xmalloc.

Or just use stack allocated VLAs:

char tmpciph[ks], tmpplain[ks];

jimis · 2017-07-06T21:29:59Z

cf-keycrypt/cf-keycrypt.c

+            if ((size = RSA_private_decrypt(ks, tmpciph, tmpplain, key, RSA_PKCS1_PADDING)) == -1)
+            {
+                fprintf(stderr, "%s\n", ERR_error_string(ERR_get_error(), NULL));
+                return -1;


File descriptor and memory leaks in case of error return. Not sure if this is a blocker though, given the short lifetime of the program.

jimis · 2017-07-06T21:35:42Z

cf-keycrypt/cf-keycrypt.c

+        usage();
+        exit(1);
+    }
+    exit(0);


Replace all zero and ones with EXIT_SUCCESS and EXIT_FAILURE. I would also suggest to just use return retval just at the end, if it's easy.

olehermanse · 2017-07-07T07:54:36Z

@jonhenrik13 We are adding cf-keycrypt to core :) You should probably take a look at this PR, it's not done yet though.

olehermanse · 2017-09-25T18:16:26Z

@jimis @nickanderson Commenting here since https://tracker.mender.io/browse/CFE-2613 has the annoying Jira+Zendesk bug.

Syntax will be:

cf-keycrypt --encrypt --keyfile /path/to/key.pub --output FILENAME.crypt FILENAME
cf-keycrypt -e -k /path/to/key.pub FILENAME > FILENAME.cfcrypt
cf-keycrypt --encrypt --keyfile /path/to/key.pub FILENAME > FILENAME.cfcrypt
cf-keycrypt --encrypt --host hostname(from cf-key -s) FILENAME > FILENAME.cfcrypt
cf-keycrypt --decrypt --keyfile /path/to/priv -o FILENAME.crypt FILENAME

Only note here is that the current implementation doesn't support piping output to stdout, so I will have to add that.

jimis · 2017-09-26T11:12:32Z

Syntax looks good to me. @nickanderson agree? Yes if `-o` is missing, output should be always on stdout, I believe it makes sense. BUT I see some conflicts with our `Log()` function, that always logs to stdout.

…

On Mon, Sep 25, 2017 at 8:16 PM, Ole Herman Schumacher Elgesem < ***@***.***> wrote: @jimis <https://github.com/jimis> @nickanderson <https://github.com/nickanderson> Commenting here since https://tracker.mender.io/browse/CFE-2613 has the annoying Jira+Zendesk bug. Syntax will be: cf-keycrypt --encrypt --keyfile /path/to/key.pub --output FILENAME.crypt FILENAME cf-keycrypt -e -k /path/to/key.pub FILENAME > FILENAME.cfcrypt cf-keycrypt --encrypt --keyfile /path/to/key.pub FILENAME > FILENAME.cfcrypt cf-keycrypt --encrypt --host hostname(from cf-key -s) FILENAME > FILENAME.cfcrypt cf-keycrypt --decrypt --keyfile /path/to/priv -o FILENAME.crypt FILENAME Only note here is that the current implementation doesn't support piping output to stdout, so I will have to add that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2883 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADayAiFyaL4J5g6Z-8Td_ypB3l0-7jvvks5sl-3-gaJpZM4OPCeD> .

This was copied from https://github.com/cfengineers-net/cf-keycrypt Moved into core by Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech> Changelog: Title Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

olehermanse · 2017-09-28T17:28:04Z

Rebased on latest master.

olehermanse · 2017-09-28T18:20:17Z

@jimis I slightly disagree with the syntax. My proposal is:

$ cf-keycrypt --encrypt message.txt --key /path/to/key --output message.enc
$ cf-keycrypt --decrypt message.enc -H somehost --output message.txt

It is almost the same, but input file must follow --encrypt or --decrypt, since that is the file that will be encrypted/decrypted. It also allows you to structure the command like a normal sentence, for example:

Using key X encrypt message.txt to message.enc: --key X --encrypt message.txt -o message.enc
Decrypt message.enc to message.txt using key X: --decrypt message.enc -o message.txt --key X

This structure makes the most sense to me.

olehermanse · 2017-09-28T21:15:45Z

I have implemented the new syntax, as explained in the previous comment.

The new help menu looks like this:

$ ./cf-keycrypt/cf-keycrypt
Usage: cf-keycrypt [OPTIONS]

Options:
  --help        , -h       - Print the help message
  --manpage     , -M       - Print the man page
  --encrypt     , -e value - Encrypt file
  --decrypt     , -d value - Decrypt file
  --key         , -k value - Use key file
  --host        , -H value - Encrypt for host (get key from lastseen database)
  --output      , -o value - Output file

Website: http://www.cfengine.com
This software is Copyright (C) 2008,2010-present CFEngine AS.

Also did a bunch of clean up Squashed commits: cf-keycrypt: Changed to spaces for indentation cf-keycrypt: Added Makefile cf-keycrypt: Added sys.cf-keycrypt variable(component) cf-keycrypt: Added automake subfolder cf-keycrypt: Added binaries to .gitignore cf-keycrypt: Added Makefile to configure.ac cf-keycrypt: Coding style cf-keycrypt: Fixed unused and uninitialized variables Changelog: Title

Squashed commits: cf-keycrypt: Fixes to acceptance test cf-keycrypt: Added to testall cf-keycrypt: Added keys and expected output cf-keycrypt: add seperate encryption and decryption tests Fixup encryption and decryption tests Really fix the encryption and decryption tests Expect encryption does not produce the same output cf-keycrypt: Add Example usage Fix example All the example pllicy blocks must be within the being and end src tags. Convert to static example

olehermanse · 2017-09-29T00:16:21Z

@jimis Please review, I believe this is ready to merge. Will need to add an enterprise and buildscripts repo for it to be packaged.

Changed the syntax as we discussed. I also did a lot of refactoring and error checking. Did not do big changes to internals like:

Change crypto.c or cf3.defs.h
Change cf-keycrypt to use getnameinfo() or Hostname2IPString()

@nickanderson Maybe look over the commit messages/changelog entries before merge.

nickanderson · 2017-09-29T00:30:05Z

This looks really good. I had to look hard to find some nit picks.

Added sys.cf-keycrypt component variable I think should be Added sys.cf_keycrypt component variable since - is not a valid character to have in a variable name.

Does it output to stdout? if -o isnt' provided? I saw in the comments that wasn't present. I wouldn`t stop this for it, just curious.

olehermanse · 2017-09-29T01:32:40Z

@nickanderson No, I removed the stdout and stdin functionality, mainly because it doesn't play well with our Log functionality.

jimis · 2017-09-18T16:04:55Z

tests/acceptance/00_basics/cf-keycrypt/decrypt.cf

Nice, acceptance tests! But the tool needs its own directory, it can't go under 00_basics. Try 27_cf-keycrypt.

Yes, @nickanderson, what were you thinking? ;)

jimis · 2017-09-29T10:49:51Z

It is almost the same, but input file must follow --encrypt or --decrypt, since that is the file that will be encrypted/decrypted. It also allows you to structure the command like a normal sentence, for example:

Using key X encrypt message.txt to message.enc: --key X --encrypt message.txt -o message.enc

Decrypt message.enc to message.txt using key X: --decrypt message.enc -o message.txt --key X

I believe this exact syntax you are describing will still be valid if [filename] is an optional argument to cf-keycrypt and --encrypt and --decrypt do not accept any argument. So you can still write it like a sentence. (blergh :-p )

Additionally in that way, the future functionality of missing [filename] meaning encrypt stdin can be added sometime.

This structure makes the most sense to me.

To me the syntax you described in your comment on Sep 25 is more familiar as it resembles gpg and a bunch of other really old UNIX encoding/decoding tools, like compress, gzip and the likes. I'm OK merging this one if you want though, the difference is small.

olehermanse · 2017-09-29T11:13:47Z

@jimis Correct me if I'm wrong but tar, gzip and compress all require you to place options (- or --) before the filename. This is the POSIX way of using getopt.

In that case, this:

I believe this exact syntax you are describing will still be valid if [filename] is an optional argument to cf-keycrypt and --encrypt and --decrypt do not accept any argument. So you can still write it like a sentence. (blergh :-p )

is wrong.

My current implementation uses the POSIX style getopt. To implement it like you say, I'd have to ditch that and continue parsing options after filename. It's not a lot of work either way, if you strongly prefer the other way. I tried to stay POSIXLY_CORRECT as the man page calls it, and this was the best way to allow you to structure the command however you like.

jimis · 2017-09-29T13:40:09Z

Dear @olehermanse tar predates POSIX and it's very non-portable, so I'll skip that. compress, gzip and the likes are reading stdin and outputing to stdout if no filename is present, and -d option of theirs is accepting no filename, but filename is an optional argument of the whole utility, not a specific option, and that's because the utility is made to act on files. So that's what I had in mind.

To implement as I say, you only have to make sure to parse your arguments with POSIX getopt(). Then on GNU systems (and our source has __GNU_SOURCE defined) the libc library makes the magic happen. Indeed this will not happen on non-GNU systems if you use getopt() which is OK by me. But you are using getopt_long() which is GNU-only, so the that code will be always used to parse that.

So what I said is right: you will be able to use your beautiful sentence-like syntax. It's not POSIX correct though, and it's not the documented way to use it, and I will still use the beautiful syntax with filename at the end, that I'm so used to in all utilities. So everyone is happy. :-)

olehermanse · 2017-09-29T13:52:16Z

@jimis Please refrain from talking smack about the almighty tape archiver - tar. I can change the syntax to your POSIX hostile abomination. This has been noted down in your personal file. My way was enforcing POSIX compliance (in terms of option order), and clearly far superior, even though it used the GNU function.

Sarcasm aside, I'll make the fix today :)

jimis · 2017-09-29T13:58:06Z

Dear @olehermanse, It is my understanding that this issue has been resolved. I will not send any further messages.

…

On Fri, Sep 29, 2017 at 3:52 PM, Ole Herman Schumacher Elgesem < ***@***.***> wrote: @jimis <https://github.com/jimis> Please refrain from talking smack about the almighty tape archiver - tar. I can change the syntax to your POSIX hostile abomination. This has been noted down in your personal file. My way was enforcing POSIX compliance (in terms of option order), and clearly far superior, even though it used the GNU function. Sarcasm aside, I'll make the fix today :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2883 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADayAuTDNlEnsH9bZBusXJnm0Bu05W3aks5snPYRgaJpZM4OPCeD> .

Also did a lot of refactoring, error checking, memory cleaning, et.c. Squashed commits: cf-keycrypt: license cf-keycrypt: PR fixes #1 cf-keycrypt: Fixed copyright in cf-keycrypt.c cf-keycrypt: CryptoInitialize() cf-keycrypt: Implemented new syntax as discussed in PR cf-keycrypt: Acceptance test for new arguments cf-keycrypt: Print help message when no arguments are added cf-keycrypt: PR fixes #2 cf-keycrypt: PR fixes #3 cf-keycrypt: PR fixes #4 cf-keycrypt: PR fixes #5 cf-keycrypt: Changed syntax to jimis' suggestion cf-keycrypt: Moved and fixed acceptance tests Changelog: Title Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

olehermanse · 2017-10-02T13:48:34Z

@jimis The syntax is now as you wanted. Input filename can be put anywhere.

olehermanse · 2017-10-02T14:57:37Z

Started build in jenkins:
https://ci.cfengine.com/job/bootstrap-enterprise-pr/432/
https://ci.cfengine.com/job/testing-enterprise-pr/403/

@jimis I believe this is ready now, do you agree?

jimis · 2017-10-02T17:25:14Z

EDIT: ignore me

olehermanse · 2017-10-03T11:43:07Z

@jimis Green on all except one platform

jimis · 2017-10-05T15:13:44Z

The code quality looks good, and the command line too (despite the bike-shedding ;-) ).

I'll now try to review the cryptographic part.

jimis

The most important comment is the RSA encryption mode. I need feedback from other people, like @estenberg, before merging.

I omitted reviewing the decrypt() function, but I assume the same notes apply.

jimis · 2017-10-05T14:53:36Z

cf-keycrypt/cf-keycrypt.c

+    const char *pubkey_path, const char *input_path, const char *output_path)
+{
+    int ks = 0;
+    unsigned long len = 0, ciphsz = 0;


len should be size_t since it's used with fread

jimis · 2017-10-05T14:54:52Z

cf-keycrypt/cf-keycrypt.c

+        memset(tmp_ciphertext, '\0', ks);
+        len = fread(
+            tmp_plaintext, 1, ks - RSA_PKCS1_PADDING_SIZE, input_file);
+        if (len <= 0 || ferror(input_file) != 0)


better declare len here not above, as size_t. Then it's gonna be obvious that testing it for negative values is bogus. Pity that the compiler does not complain, I'm quite sure a static analyzer would catch it.

Hmm, I have a theory: if you feed cf-keycrypt a file with size RSA_size(pubkey) - RSA_PKCS1_PADDING_SIZE, then it will output an error "could not read file". This test most likely needs to be if (len == 0 && ferror()).

jimis · 2017-10-05T15:00:19Z

cf-keycrypt/cf-keycrypt.c

+            error_return = true;
+            break;
+        }
+        unsigned long size = RSA_public_encrypt(


size should be int, because that's what RSA_public_encrypt returns and because it's compared to negative numbers later on.

jimis · 2017-10-05T15:00:48Z

cf-keycrypt/cf-keycrypt.c

+            break;
+        }
+        unsigned long size = RSA_public_encrypt(
+            strlen(tmp_plaintext), tmp_plaintext, tmp_ciphertext,


shouldn't this be len? What happens if the file is binary and contains NULLs and you call strlen(tmp_plaintext) ?

jimis · 2017-10-05T15:09:09Z

cf-keycrypt/cf-keycrypt.c

+        memset(tmp_plaintext, '\0', ks);
+        memset(tmp_ciphertext, '\0', ks);
+        len = fread(
+            tmp_plaintext, 1, ks - RSA_PKCS1_PADDING_SIZE, input_file);


So it keeps reading from a file 245 byte chunks, and RSA-encrypts each and every one separately using PKCS #1 v1.5 padding, and appends to a file, and each block is fully independent from the other without any chaining whatsoever. This is definitely fishy but I am not sure if there are specific vulnerabilities. Maybe @estenberg can comment ?

@jimis you are right, this is not good because 1) it leaks information as identical input chunks result in identical encrypted data 2) encrypting large volumes with a public key can leak information about the private key and 3) it is very inefficient (compared to symmetric crypto). At the very least we need to set a hard limit on the size (e.g. you can only encrypt 245 bytes or less), so it would really only work for password strings and the like.

The "right way" to do this is of course to encrypt a random symmetric key with RSA and then use that key with a block or stream cipher in a secure mode (e.g. CBC, CTR, etc.)

jimis · 2017-10-05T15:10:09Z

cf-keycrypt/cf-keycrypt.c

+                ipaddress,
+                sizeof(ipaddress));
+            snprintf(
+                buffer, BUFSIZE * sizeof(char), "%s/ppkeys/root-%s.pub",


We don't really use this kind of line breaking elsewhere in our code, unless it is almost unavoidable. Not a blocker, just keep in mind for next time.

estenberg · 2017-10-06T22:09:56Z

Two high-level comments:

This is insecure if we are encrypting more than ~200 bytes (even then it is not great). Either we need to set a hard limit on this input size or we need to implement this "properly" with symmetric crypto.
I don't think "cf-keycrypt" is a particularly catchy name. Why not just call it cf-crypt?

olehermanse · 2017-10-09T13:30:15Z

@jimis So I don't suspect I'll have time to rewrite the encryption code in at least a month. What do you want to do? Take a stab at it yourself, or wait until november?

jimis · 2017-10-23T14:44:18Z

Not sure how to handle this, but we can't knowingly put in weak encryption. Lets bring this one up in the next planning meeting.

P.S. to do proper public key encryption seems to be a bit complicated; found this article: https://shanetully.com/2012/06/openssl-rsa-aes-and-c/

olehermanse · 2017-11-20T14:51:23Z

@jimis I also found this which is a little more up to date, but doesn't show AES part. http://hayageek.com/rsa-encryption-decryption-openssl-c/

jimis · 2017-11-20T15:11:25Z

Great @olehermanse ! I'm all for doing this right, since cf-keycrypt is a tool people ask for all the time and it's really almost done. But I'm not sure if the code in that link can encrypt arbitrarily long buffers. See man RSA_public_encrypt and you'll know what I mean.

Before coding stuff though we should make sure we are doing it the right way, maybe this is material for a meeting, or discussion on the relevant ticket.

olehermanse · 2017-11-20T16:23:50Z

@jimis I know, hence why I said it didn't cover the AES step. (RSA to encrypt AES key, AES to encrypt data)

jimis · 2018-01-29T16:32:29Z

Closing since CFE-2613 is not scheduled. Feel free to re-open when work resumes.

The codebase has evolved since the last attempt to merge cf-keycrypt in cfengine#2883.

The codebase has evolved since the last attempt to merge cf-keycrypt in cfengine#2883. (cherry picked from commit bd3e840)

olehermanse force-pushed the cf-keycrypt branch from e01cc87 to 63d34ca Compare July 6, 2017 01:21

olehermanse force-pushed the cf-keycrypt branch 4 times, most recently from 6371c59 to 6377b5b Compare July 6, 2017 10:05

olehermanse changed the title ~~Including and packaging cf-keycrypt with core.~~ Include and package cf-keycrypt with core Jul 6, 2017

olehermanse force-pushed the cf-keycrypt branch from 04e52a2 to a4c432e Compare July 6, 2017 15:28

jimis suggested changes Jul 6, 2017

View reviewed changes

nickanderson changed the title ~~Include and package cf-keycrypt with core~~ CFE-2613: Include and package cf-keycrypt with core Jul 6, 2017

jimis suggested changes Jul 6, 2017

View reviewed changes

jimis reviewed Jul 6, 2017

View reviewed changes

olehermanse force-pushed the cf-keycrypt branch from 8b5742b to 95d85bb Compare July 13, 2017 06:38

jimis self-assigned this Sep 20, 2017

Moved the cf-keycrypt utility to CFEngine core

c3f66cb

This was copied from https://github.com/cfengineers-net/cf-keycrypt Moved into core by Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech> Changelog: Title Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

olehermanse force-pushed the cf-keycrypt branch from a07e63e to df6ea7f Compare September 28, 2017 17:27

olehermanse force-pushed the cf-keycrypt branch from fba7bc5 to f975a28 Compare September 28, 2017 23:53

olehermanse and others added 2 commits September 29, 2017 02:05

olehermanse force-pushed the cf-keycrypt branch from f975a28 to 332d302 Compare September 29, 2017 00:11

jimis reviewed Sep 29, 2017

View reviewed changes

olehermanse force-pushed the cf-keycrypt branch from 332d302 to 6f508ae Compare October 2, 2017 12:37

olehermanse force-pushed the cf-keycrypt branch from 6f508ae to 509f825 Compare October 2, 2017 13:26

olehermanse mentioned this pull request Oct 2, 2017

Added cf-keycrypt to buildscripts cfengine/buildscripts#261

Closed

jimis reviewed Oct 5, 2017

View reviewed changes

jimis closed this Jan 29, 2018

vpodzime added a commit to vpodzime/cfengine-core that referenced this pull request Feb 13, 2020

Adapt cf-keycrypt source to changes in the codebase

bd3e840

The codebase has evolved since the last attempt to merge cf-keycrypt in cfengine#2883.

vpodzime added a commit to vpodzime/cfengine-core that referenced this pull request Jun 5, 2020

Adapt cf-keycrypt source to changes in the codebase

650f533

The codebase has evolved since the last attempt to merge cf-keycrypt in cfengine#2883. (cherry picked from commit bd3e840)

CFE-2613: Include and package cf-keycrypt with core #2883

CFE-2613: Include and package cf-keycrypt with core #2883

Uh oh!

Conversation

olehermanse commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olehermanse commented Jul 6, 2017

Uh oh!

jimis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimis commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olehermanse commented Jul 6, 2017 •

edited

Loading

olehermanse commented Sep 29, 2017 •

edited

Loading