autolab / autolab-cli Goto Github PK

$ autolab submit -f 15213-s23:attacklab test.txt
fatal: Invalid command line argument 'test.txt'.
Note that all options must come after all positional arguments (e.g. commands).
For detailed usage, run with '-h'.

This is an extremely strange thing for a Unix command line utility to insist on. I've been mucking around with Unix for 35 years and the only other examples I can think of are find (where the stuff that comes after the positional arguments is more accurately described as an expression in a mini-language, not options) and ld (where everyone agrees that it sucks that you have to put the -l options at the end of the link line).

Please change this to be more normal, by which I mean "options can appear in any order and in any position relative to the positional parameters." It's OK if "submit" has to come before options that are specific to the submit subcommand, but I should be allowed to do what I did in the example at the top.

If the autolab cli's encryption key for the .arcache file does not match what was used to write it, all that's displayed is openssl's error (which is even less than useful because openssl's error strings are not loaded). The tool should either diagnose the mismatch and describe it, or proactively discard the cache

error:06065064:lib(6):func(101):reason(100):evp_enc.c:592:

/tmp/autolab-cli/src/file/file_utils.cpp:118:17: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
   while (amount = TEMP_FAILURE_RETRY(read(fd, result + total_read, max_length))) {
                 ^
cc1plus: all warnings being treated as errors

This does not occur in older versions of GCC, so I assume that's why this wasn't caught sooner.

It seems a little silly, but simply adding another pair of parentheses around that while condition will satisfy GCC. I found that a bit distasteful, so instead I added -Wno-error=parentheses to CMakeLists.txt on this line:

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra -Werror -Wno-error=parentheses")

At least one of these changes should be made, or else the client won't compile on recent versions of GCC.

Occurring on AWS Autolab.

Error

pygmyshark:/afs/cs/academic/class/18213-m23/private/Autolab-CLI/build% src/autolab setup
read size 96
fatal: OpenSSL error
001E482E667F0000:error:1C800064:Provider routines:ossl_cipher_unpadblock:bad decrypt:../providers/implementations/ciphers/ciphercommon_block.c:124:

Fix

/afs/cs/academic/class/18213-m23/private/Autolab-CLI/src/crypto/pseudocrypto.cpp:
std::string decrypt_string(char *srctext, size_t srclength, unsigned char *key,
    unsigned char *iv) {
  check_key_and_iv_lengths(key, iv);
  EVP_CIPHER_CTX *ctx;
  unsigned char plaintext[MAX_CIPHERTEXT_LEN];
  int total_len = 0;
  int temp_len = 0;
  unsigned char *ciphertext = (unsigned char *)srctext;
  int input_len = (int)srclength;
  if (!(ctx = EVP_CIPHER_CTX_new()))
    exit_with_crypto_error();
  EVP_CIPHER_CTX_set_padding(ctx, 0); // GMK
  if (1 != EVP_DecryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, key, iv))
    exit_with_crypto_error();

Potential Explanation

I think the problem may be related to encrypt_string(...) at pseudocrypto.cpp:95:
if (1 != EVP_EncryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, key, iv))
exit_with_crypto_error();
I think EVP_aes_256_cbc() may be a legacy encoding. I don't know exactly how it pads, but I don't think it will add a block if ((SIZE/BLOCK_SIZE)==0).
The decryption with padding uses padding a bit like a checksum. It expects each block to be padded with N bytes of value N, where N is the number of bytes needed to fill out the 256-byte block. So, for example, if the last block in the message has 250 bytes, 5 bytes would be added, each with a value of 0x5. But, to make this work there needs to be a checksum even if the data size is a multiple of the block size, so, in this case, it expects that, during the encryption, an entirely empty block, fully padded with 256 0xFF bytes, was added to the end of the message. It is this step that I don't think is being performed by the legacy encoding used in the encrypt_string(...) function.
The result is that it looks at the last byte of the last block it gets and expects to see the number of bytes of padding in the block, but it sees, instead, the last byte of data. I think it then expects to find exactly that many bytes of exactly that value as the tail values of the block, but instead finds a bunch of arbitrary values, ie. the last bytes of the actual data.
So, I think we can either turn off the check on padding or switch encryption to a more modern one. But, if we don't I think that, at least, 1 time out of 256 (and maybe every time), this error will occur.
I haven't dug into this deeply. But, this is my hot take from a quick look at the encrypt_string() and decrypt_string() functions.
Maybe this helps?

We observed this in 2 situations:
        1. when trying to download a non-existing assignment.
        2. when trying to download an already downloaded assignment.
           It's great that the CLI refuses to download the assignment if a directory
           with that name already exists, but it returns the same exit code as a
           successful download (which throws out our script).
        (There may be other situations, but we haven't run into them.)

https://github.com/autolab/autolab-cli/blob/master/src/cmd/cmdimp.cpp

Currently autolab submit insists on being given the name of a file that both exists and is a "regular file" (file_exists is true only for files that pass stat(file, &st) == 0 && S_ISREG(st.st_mode)). This means it is impossible to get it to read the submission from standard input, which makes it significantly harder to feed a submission to autolab submit from a C program.

Please make at least one, preferably both, of the following changes:

1. If the file name given on the command line is a single ASCII minus sign (-), read the submission from standard input. (Skip the "does this file exist" check in [], and then all the way down in RawClient::raw_request, change how you call curl_formadd.)
2. Allow use of /dev/stdin as the file argument, by changing file_exists so that it considers files that exist but aren't regular files, as long as you can read() from them normally. (Probably the right thing is to blacklist S_ISDIR and S_ISBLK and accept anything else.)

Also, whichever of the two changes you make, you need to test that submitting standard input works correctly when stdin is any of:

• a file that is open but unlinked (e.g. what you get from tmpfile())
• a pipe
• a stream-oriented socket (testing socketpair(AF_LOCAL, SOCK_STREAM, 0) sockets should be good enough)
• a tty

These are in decreasing order of importance, but ideally all of them would work.

When the first task does not have associated feedback, the CLI reports an error instead of looking for the feedback of tasks that do have associated feedback. See screenshot.

Here "--" for task 1 is normal because this task will be graded after the submission deadline.

Screenshot:

When using
autolab setup

Authorization shows as received, and that the user setup is complete.
However, when using autolab status, the client reports that no user is setup. Looking at the web ui, no authorized applications are displayed.

Currently running autolab version 2.8.0

I tried running "autolab course" (forgetting the 's' in "courses") and received the following error:

[ContextManager] tokens loaded
fatal: Cannot start autolab client
Authorization invalid or expired.

Please re-authorize this client by running 'autolab-setup'

This works when running any subcommand that isn't valid. It's not disastrous, but it is misleading.

Do I have to make? and what is the meaning of that?

Occurring on AWS Autolab.

"The CLI seems to need to be reauthenticated to the server by running "setup" and copying over the code every ~3 wall clock hours."

Exception should be thrown via main > show_courses > get_courses > make_request > raw_request_optional_refresh

Possible root cause is RawClient::perform_token_refresh

In README.md the Getting Client Credentials section mentioned that we need to register a new API application. The text mentioned what we should use in the Redirect URI field. But it does not mention what to do with the Scopes field, and apparently that is important.

In the rails app I dug around and found the scopes defined in config/initializers/doorkeeper.rb and with some trial and error discovered that these values in the scopes field seem to be sufficient for the autolab-cli to function:

user_info user_courses user_scores user_submit

Is this correct? If so, I suggest updating README.md for the benefit of future users.

autolab status

Due: Sat Jul 25 21:00:00 2020
Max submissions: Infinite
Max grace days: 1
It shows "Max submissions: Infinite". This is extremely misleading to students.
Could you change the output to display the number of unpenalized submissions.
In this case, it should display "Unpenalized submissions left: 9".

A transcript of a session I just had:

rzachari@unix4:~/private/15122$ autolab122 download pixels
Querying assessment 'pixels' of course '15122-n20' ...
fatal: User is not in this course
chmod: cannot access ‘pixels/.autolab-asmt’: No such file or directory
tar (child): pixels/pixels-handout.tgz: Cannot open: No such file or directory
tar (child): Error is not recoverable: exiting now
tar: Child returned status 2
tar: Error is not recoverable: exiting now
mv: cannot stat ‘pixels-handout/*’: No such file or directory
rmdir: failed to remove ‘pixels-handout’: No such file or directory
pixels successfully downloaded.

(The last line is wrong.)

I got the following error when trying to build on a Mac M1.

Any ideas on how to fix it?

Best,

Alex

alexwadell@Alexs-MBP build % git status   
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
alexwadell@Alexs-MBP build % git rev-parse HEAD
57713f2b1ac59bd1d2ac7602c36b4cbf7c1cb1e5
alexwadell@Alexs-MBP build % cmake .. -Drelease=ON
CMake Deprecation Warning at CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.


-- Build type: Release
-- Build variant: 
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/alexwadell/Autolab-CLI/build
alexwadell@Alexs-MBP build % gmake --version
GNU Make 4.3
Built for arm-apple-darwin20.2.0
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
alexwadell@Alexs-MBP build % gmake
[  5%] Building CXX object lib/logger/CMakeFiles/logger.dir/logger.cpp.o
In file included from /Users/alexwadell/Autolab-CLI/lib/logger/logger.cpp:1:
/Users/alexwadell/Autolab-CLI/lib/logger/./logger.h:76:32: error: unused parameter 'val' [-Werror,-Wunused-parameter]
    debug_logger &operator<<(T val) {
                               ^
1 error generated.
gmake[2]: *** [lib/logger/CMakeFiles/logger.dir/build.make:82: lib/logger/CMakeFiles/logger.dir/logger.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:285: lib/logger/CMakeFiles/logger.dir/all] Error 2
gmake: *** [Makefile:149: all] Error 2
alexwadell@Alexs-MBP build % gcc --version
Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.28)
Target: arm64-apple-darwin20.2.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin