scitokens-verify change breaks compatibility

[DrDaveD]

#159 will cause existing scripts that use scitokens-verify (including htdecodetoken) to break.

Instead, the old form should be supported in addition to the new, at least for a transition period. It could print a deprecation warning if the old form is used.

[djw8605]

I believe the old form and the new are both supported. At least that's what I wrote and tested. Is it broken?

[djw8605]

It should take the token on both the command line and stdin, but it will give a warning (to stderr) if you provide the token on the command line:

scitokens-cpp/src/verify.cpp

Lines 90 to 101 in 89257bb

	std::string token;
	// If a positional argument is present, treat it as the token (with warning)
	if (optind < argc) {
	fprintf(stderr,
	"%s: Warning: Providing the token on the command line is "
	"insecure. Please use stdin instead.\n",
	argv[0]);
	token = argv[optind];
	} else {
	// Read token from stdin
	std::getline(std::cin, token);
	}

[DrDaveD]

Oh I hadn't tried it, I was just going by how it disappeared from the usage.

I tried it just now on el8 and I'm getting a free(): invalid pointer error when it cleans up. Gdb gives his stack trace:

#0  0x00007ffff5fb252f in raise () from /lib64/libc.so.6
#1  0x00007ffff5f85e65 in abort () from /lib64/libc.so.6
#2  0x00007ffff5ff3727 in __libc_message () from /lib64/libc.so.6
#3  0x00007ffff5ffaa2c in malloc_printerr () from /lib64/libc.so.6
#4  0x00007ffff5ffc5b4 in _int_free () from /lib64/libc.so.6
#5  0x00007ffff74d1bca in BN_free () from /lib64/libcrypto.so.1.1
#6  0x00007ffff7bb8279 in std::unique_ptr<bignum_st, void (*)(bignum_st*)>::~unique_ptr (this=<synthetic pointer>, __in_chrg=<optimized out>)
    at /usr/include/c++/8/bits/unique_ptr.h:350
#7  (anonymous namespace)::rs256_from_coords (
    n_str="<redacted>"..., e_str="AQAB")
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens_internal.cpp:481
#8  scitokens::Validator::get_public_key_pem_continue (
    status=std::unique_ptr<scitokens::AsyncStatus> = {...}, public_pem="",
    algorithm="")
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens_internal.cpp:918
#9  0x00007ffff7bb985b in scitokens::Validator::get_public_key_pem (
    issuer="https://cilogon.org/fermilab",
    kid="B8634951FD1E315DAF75C94AECF03668", public_pem="", algorithm="")
    at /usr/include/c++/8/bits/move.h:74
#10 0x00007ffff7bb1ffc in scitokens::Validator::verify_async (
    this=this@entry=0x7fffffffcd80, jwt=...)
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens_internal.h:510
#11 0x00007ffff7bba415 in scitokens::Validator::verify (jwt=...,
    this=0x7fffffffcd80)
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens_internal.h:436
#12 scitokens::SciToken::deserialize (this=this@entry=0x634300,
data="<redacted>"...,
    allowed_issuers=std::vector of length 0, capacity 0)
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens_internal.cpp:600
#13 0x00007ffff7b9edc9 in scitoken_deserialize_v2 (
    value=0x635ee0 "<redacted>"...,
    token=0x634300, allowed_issuers=<optimized out>, err_msg=0x7fffffffd030)
    at /usr/include/c++/8/ext/new_allocator.h:79
#14 0x00007ffff7b9f2f2 in scitoken_deserialize (
    value=0x635ee0 "<redacted>"...,
    token=token@entry=0x7fffffffd010,
    allowed_issuers=allowed_issuers@entry=0x0,
    err_msg=err_msg@entry=0x7fffffffd030)
    at /nashome/d/dwd/work/scitokens-cpp/src/scitokens.cpp:288
#15 0x0000000000401447 in main (argc=<optimized out>, argv=0x7fffffffd328)
    at /usr/include/c++/8/bits/basic_string.h:2294

[DrDaveD]

Since a deserialization failure prints to stdout, I would prefer the deprecation warning to go there too. That way people will only see the message if there's a failure. Is that alright with you?

[DrDaveD]

Ah, the crash doesn't happen on el9, only el8. On el8 there's also a compilation warning that may be relevant:

/home/dwd/work/scitokens-cpp/src/scitokens_internal.cpp: In function 'std::unordered_map<std::__cxx11::basic_string<char>, jwt::basic_claim<jwt::traits::kazuho_picojson> > json_to_claim_map(const object&)':
/home/dwd/work/scitokens-cpp/src/scitokens_internal.cpp:586:22: warning: structured bindings only available with -std=c++17 or -std=gnu++17
     for (const auto &[name, value] : json) {
                      ^

I can make the warning disappear by putting CCFLAGS=-std=c++17 in the generated Makefile but it still crashes.

[djw8605]

It wasn't related to the c++17 thing, with the help of git bisect, I found it to be fixed in #162

[DrDaveD]

Ok the remaining issue is that I would like the new warning message to go to stdout, because htdecodetoken only shows that when there's an error and I don't want this message to show up every time somebody runs htdecodetoken.