Allow for more TOTP credentials and enable cycling backwards through codes

[maxz]

When settings up and using the TOTP face I came across some warts.

I increased the rather low limit of credentials which I ran into (previously around 13 with a key size of 20 bytes) to above 3000. Additionally it now is possible to cycle backwards through the codes by pressing the light button.

Those two things were in my local copy anyways, but I thought other people might appreciate them as well.
And since I wanted to submit these patches, I was motivated to tackle the rather repetitive configuration of the face too. When changing credentials and moving them around I found it rather frustrating to keep all the arrays in sync.
The algorithm now defaults to SHA1, the timestep to 30 seconds and the key size to 20.
Those were the values which occurred most often in my TOTP credentials.
The number of arrays was reduced to two, which was a concession I had to make to avoid malloc and storage waste. (If I had chosen a static array for the keys, I would probably have picked a size of 40, since the biggest one of my keys is 35 bytes. But there is no key size limit in the standard and there are ridiculously big keys (128 bytes) in the wild.)

[sntyj]

I think if you aren't using EVENT_LIGHT_LONG_PRESS for anything, that adding LED functionality would at least prevent losing a feature. Something like this from the thermistor sensor:

        case EVENT_LIGHT_LONG_PRESS:
            movement_illuminate_led();
            break;

Your comment said

The light could also be activated at the same time, but I decided
against this since the cases where someone would require their TOTP
credentials while relying on the LED light to read them seemed very
unlikely to occur.

I can tell you as a dad trying to put their kid down at night, but trying to log into something on their phone with a credential on their watch, that I use this feature frequently :)

EDIT: To specify, I'm looking at movement/watch_faces/complication/totp_face.c on line 142.

EDIT 2: Updated quote so I'm not misrepresenting author comment.

[maxz]

Using a long press for the light seems reasonable to me.
It keeps the functionality and does not interfere with the backward cycling.

I will change that. Thanks for your feedback and also for the reference of how the other face did it. I really though it to be unlikely for someone to use this. Whenever there would be some kind of screen I would have expected it to provide enough light to read the watch.

[sntyj]

Tested in emulator with my current TOTP config was successful, and worked as I expected. I've only commented on small documentation things now, but functionally I think the code is sound and easier to understand than before.

[maxz]

Thank you. The missing comment was an oversight.

[sntyj]

Looks great, and good work!

[sntyj]

@WesleyAC or @joeycastillo will probably want to take a look and they can merge this in.

[maxz]

I provide all my changes under the MIT license which the rest of this project uses right now. (To avoid someone having to explicitly ask like in #334.)

I did not feel like adding a license text to the individual file would add anything. By not adding it I'm implicitly using the project license anyways.

[maxz]

I squashed all the various smaller corrections together and properly separated the commit with the credential changes from the commit with the backwards cycling changes.

[maxz]

I had a previous version of these changes already running on hardware and these changes also worked fine in the simulator, but now I deployed exactly these commits to hardware and they still work as expected.

And as a motivation for the changed format, see the following comparison of my credentials in the new style and the old one.
The keys arrays are identical for both.

#pragma GCC diagnostic ignored "-Winitializer-overrides"
const static totp_parameters_t credentials[] = {
    CREDENTIAL(.label = "GO"),
    CREDENTIAL(.label = "MX"),
    CREDENTIAL(.label = "AN", .key_size = 32),
    CREDENTIAL(.label = "IN"),
    CREDENTIAL(.label = "ST"),
    CREDENTIAL(.label = "GH", .key_size = 10),
    CREDENTIAL(.label = "GL"),
    CREDENTIAL(.label = "UU"),
    CREDENTIAL(.label = "SH", .key_size = 10),
    CREDENTIAL(.label = "CB", .key_size = 40),
    CREDENTIAL(.label = "JB", .key_size = 10),
    CREDENTIAL(.label = "BB"),
    CREDENTIAL(.label = "EB"),
    CREDENTIAL(.label = "EA", .key_size = 10),
    CREDENTIAL(.label = "VF"),
    CREDENTIAL(.label = "TE"),
    CREDENTIAL(.label = "DC", .key_size = 10),
    CREDENTIAL(.label = "PL", .key_size = 10),
    CREDENTIAL(.label = "WB", .key_size = 40),
};

static const uint8_t num_keys = 19;
static const uint8_t key_sizes[] = {
    20,
    20,
    32,
    20,
    20,
    10,
    20,
    20,
    10,
    40,
    10,
    20,
    20,
    10,
    20,
    20,
    10,
    10,
    40,
};
static const uint32_t timesteps[] = {
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
    30,
};
static const char labels[][2] = {
    { 'G', 'O' },
    { 'M', 'X' },
    { 'A', 'N' },
    { 'I', 'N' },
    { 'S', 'T' },
    { 'G', 'H' },
    { 'G', 'L' },
    { 'U', 'U' },
    { 'S', 'H' },
    { 'C', 'B' },
    { 'J', 'B' },
    { 'B', 'B' },
    { 'E', 'B' },
    { 'E', 'A' },
    { 'V', 'F' },
    { 'T', 'E' },
    { 'D', 'C' },
    { 'P', 'L' },
    { 'W', 'B' },
};
static const hmac_alg algorithms[] = {
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
    SHA1,
};

[matheusmoreira]

Hello! I see you have lifted the arbitrary limitation by increasing the size of the offset variable. I submitted PR #369 which renders that variable obsolete. I implemented proper structured access to the records. The offsets are implied by the data and array structures and so it is no longer necessary to manually keep track of them.

It works like this:

static uint8_t key_1[] = {
    0x48, 0x65, 0x6c, 0x6c, 0x6f, 0x21, 0xde, 0xad, 0xbe, 0xef, // 1 - JBSWY3DPEHPK3PXP
};

static uint8_t key_2[] = {
    0x48, 0x65, 0x6c, 0x6c, 0x6f, 0x21, 0xde, 0xad, 0xbe, 0xef, // 2 - JBSWY3DPEHPK3PXP
};

static totp_t totp_data[] = {
    TOTP_INITIALIZER('2', 'F', key_1, SHA1, 30),
    TOTP_INITIALIZER('A', 'C', key_2, SHA1, 30),
};

static inline totp_t *_totp_current(totp_state_t *totp_state) {
    return &totp_data[totp_state->current_index];
}

static inline size_t _totp_num(void) {
    return sizeof(totp_data) / sizeof(*totp_data);
}

Would you like to work together to merge our improvements?

[maxz]

Sure. I'd suggest you rebase your changes on top of my branch and then post your patches (in full git format, with authorship information) or a link to your branch here and I add the changes to my branch.

Then we can go from there.
I'd generate the key arrays as part of a macro (otherwise there is not much gained over the current offset based variant) and use the label or something alike as the name for the array.
And you should modify the struct and macro in totp.h instead of defining new ones in totp.c.

I don't have much time in the next ~1 week but will take a look afterwards and make changes or suggestions.

[matheusmoreira]

@maxz

I'd generate the key arrays as part of a macro

Implemented.

static totp_t credentials[] = {
    CREDENTIAL(2F, "JBSWY3DPEHPK3PXP", SHA1, 30),
    CREDENTIAL(AC, "JBSWY3DPEHPK3PXP", SHA1, 30),
};

It will even warn the user if the symbol is too long!

CC build-sim/totp_face.o
totp_face.c:62:5: warning: initializer-string for char array is too long [-Wexcess-initializers]
   62 |     CREDENTIAL(GITHUB, "XXXXXXXXXXXXXXXX", SHA1, 30),
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
totp_face.c:54:19: note: expanded from macro 'CREDENTIAL'
   54 |         .labels = (#label), \
      |                   ^~~~~~~~
1 warning generated.

use the label or something alike as the name for the array

They are now anonymous.

[matheusmoreira]

I have incorporated commit 055d9c9 into my branch and am now running it on real hardware. No issues so far.

[maxz]

I'm not sure what to say about this now that your changes got merged. I'm sure you meant well and most of your changes were good, but you also dropped essential parts of my changes. You just merge the easy cycling commit and dropped the changes which would still improve the definition of new credentials in the new state of the main branch. I guess I will just resubmit them and hope that this time nobody comes along who changes things differently and then pesters the maintainers until they merge it.

[matheusmoreira]

I apologize if what I did was inappropriate. It was not my intention to offend you. It's just that you said you weren't going to have much time to focus on this so I went ahead and tried to do as much as I could.

I thought it was best to reduce the scope of the changes due to the feedback I received in my clock face refactoring PR. Too many changes made it difficult to review, I had to remove some features. That's why I didn't integrate your complete change set. I will be merging more PRs in the next branch and I plan to revisit this one. The Python script in particular can improve things even further and also reduce memory usage.

I also took your suggestions seriously: I came up with a macro that makes it easy and safe to define credentials, and used compound literals in order to avoid the need to name the arrays to begin with or even have the arrays at all. I did not follow your suggestion to move the structure to the header but I had a good reason for that: no code outside that file references the structure, making it implicitly implementation internal.

Still, your work has been merged into the main branch, which should be cause for celebration. It was only slightly changed for integration purposes but I ensured you retained authorship, made sure to fully credit you in multiple places, and also fully complied with all terms of the MIT license under which we are releasing our code. I even updated the copyrights to include everyone who ever contributed to the file. So I'm not sure what the problem is. If there's something I forgot to do, simply point it out and I will get it done.

I don't appreciate you calling me a pest. I'm new here but I'm very enthusiastic about this project and I really want to see it succeed. It's true that I was anxious to see the improvements merged. However, that was due to enthusiasm, not entitlement. I actually asked a couple of times in the discord about this: I was worried my actions were excessive but nobody really said anything. Surely atax1a would have told me to stop pestering them if that was the case. I would have stopped had they told me to and I gave the maintainers ample opportunity to do so. Hell, I'm giving them that oppotunity right now, in public.

[maxz]

I also took your suggestions seriously: I came up with a macro that makes it easy and safe to define credentials, and used compound literals in order to avoid the need to name the arrays to begin with or even have the arrays at all. I did not follow your suggestion to move the structure to the header but I had a good reason for that: no code outside that file references the structure, making it implicitly implementation internal.

That is true and I appreciate it.

So I'm not sure what the problem is. If there's something I forgot to do, simply point it out and I will get it done.

The biggest thing that I'm missing are the default values which I had defined for the CREDENTIAL macro.
They would make the credentials even easier to manage and together with your changes I think they'd even make the Python program pretty much obsolete unless I missed something while looking at the changes.

I don't appreciate you calling me a pest. I'm new here but I'm very enthusiastic about this project and I really want to see it succeed. It's true that I was anxious to see the improvements merged. However, that was due to enthusiasm, not entitlement.

I'm sorry. I did not mean it that harsh. I was previously not aware of pest being a possible root of the word pestering.

You got to try to see how I felt. I patiently waited for weeks for someone to review my changes. And then you came along, made changes to the same place and expedited the process by opening multiple issues and merging the things.
I really appreciate your commitment but it also felt to me like I could have spent the time it took me to write and refine my commits with doing something else.

[matheusmoreira]

The biggest thing that I'm missing are the default values which I had defined for the CREDENTIAL macro. They would make the credentials even easier to manage

Do you mean the ability to omit the algorithm and period? Like this:

static totp_t credentials[] = {
    CREDENTIAL(2F, "JBSWY3DPEHPK3PXP"),
    CREDENTIAL(AC, "JBSWY3DPEHPK3PXP"),
};

If so I agree with you. I thought your approach was quite clever too but in the end I opted for the shorter functional syntax. What do you think of defining an additional function for the defaults?

static totp_t credentials[] = {
    DEFAULT_CREDENTIAL(2F, "JBSWY3DPEHPK3PXP"),
    DEFAULT_CREDENTIAL(AC, "JBSWY3DPEHPK3PXP"),

    CREDENTIAL(TO, "JBSWY3DPEHPK3PXP", SHA1, 30),
    CREDENTIAL(R2, "JBSWY3DPEHPK3PXP", SHA1, 30),
};

Or maybe:

static totp_t credentials[] = {
    CREDENTIAL(2F, "JBSWY3DPEHPK3PXP"),
    CREDENTIAL(AC, "JBSWY3DPEHPK3PXP"),

    FULL_CREDENTIAL(TO, "JBSWY3DPEHPK3PXP", SHA1, 30),
    FULL_CREDENTIAL(R2, "JBSWY3DPEHPK3PXP", SHA1, 30),
};

The default macro can be defined in terms of the full macro.

together with your changes I think they'd even make the Python program pretty much obsolete unless I missed something while looking at the changes

Your script improves the code. I achieved the lack of arrays by placing the base 32 string in the ROM and decoding it to RAM when initializing the watch face at runtime. The script pre-computes them and generates the code, freeing up the memory.

Unfortunately the macros aren't powerful enough to pre-compute the arrays. I wish C had a Zig-like comptime thing.

it also felt to me like I could have spent the time it took me to write and refine my commits with doing something else

I apologize for that too. Truth is I started writing all these PRs without checking whether the features I wanted were already being worked on. I too ended up wasting a bunch of time. I fixed that though by going through every PR and familiarizing myself with them. I'm also watching this repository for new PRs. It shouldn't happen again.

[maxz]

The biggest thing that I'm missing are the default values which I had defined for the CREDENTIAL macro. They would make the credentials even easier to manage

Do you mean the ability to omit the algorithm and period?

Exactly.

If so I agree with you. I thought your approach was quite clever too but in the end I opted for the shorter functional syntax. What do you think of defining an additional function for the defaults?

I'm not too fond of the first suggestion, at least not if the name is longer. I think most people who were not involved in writing any of this would then just use CREDENTIAL, while the version with the default values should have no downside apart from relying on values which have to be communicated (e. g. in the documentation).

I think if the explicit version is to be kept, I'd name it EXPLICIT_CREDENTIALS, FULL_CREDENTIALS or something alike, basically as a "low level" version. And then have CREDENTIALS which uses the default values and is used in all the documentation. That would lead to the shortest configuration without the possible verbosity in the algorithm and period fields which I tried to eliminate in the first place.

Oh, I just saw that your second suggestion is pretty much what I described. So yes, that one would be what I had in mind:

static totp_t credentials[] = {
    CREDENTIAL(2F, "JBSWY3DPEHPK3PXP"),
    CREDENTIAL(AC, "JBSWY3DPEHPK3PXP"),

    FULL_CREDENTIAL(TO, "JBSWY3DPEHPK3PXP", SHA1, 30),
    FULL_CREDENTIAL(R2, "JBSWY3DPEHPK3PXP", SHA1, 30),
};

The default macro can be defined in terms of the full macro.

together with your changes I think they'd even make the Python program pretty much obsolete unless I missed something while looking at the changes

Your script improves the code. I achieved the lack of arrays by placing the base 32 string in the ROM and decoding it to RAM when initializing the watch face at runtime. The script pre-computes them and generates the code, freeing up the memory.

Yes, that was the idea behind it. Also I did not want to add the complexity of decoding the base32 string at runtime (although it is already handled similarly in the totp_face_lfs.c.)
Like you I wished for a construct which would have allowed to decode it at compile time.

[matheusmoreira]

I just remembered something. We can have a clean single function interface.

While exploring the musl codebase, I came across this wonderful piece of C preprocessor wizardry:

#define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n
#define __SYSCALL_NARGS(...) __SYSCALL_NARGS_X(__VA_ARGS__,7,6,5,4,3,2,1,0,)
#define __SYSCALL_CONCAT_X(a,b) a##b
#define __SYSCALL_CONCAT(a,b) __SYSCALL_CONCAT_X(a,b)
#define __SYSCALL_DISP(b,...) __SYSCALL_CONCAT(b,__SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
#define __syscall(...) __SYSCALL_DISP(__syscall,__VA_ARGS__)

The most important macros are __SYSCALL_NARGS and __SYSCALL_NARGS_X. The former forwards all its arguments plus 8 numbers decreasing from 7 to 0 to the latter, which returns the 9th number. The arguments push the numbers back by their count, changing the number in the 9th position in increasing order.

Here's how they're expanded:

1. __syscall(60, 0)
2. __SYSCALL_DISP(__syscall, 60, 0)
3. __SYSCALL_CONCAT(__syscall, __SYSCALL_NARGS(60, 0))(60, 0)
4. __SYSCALL_CONCAT(__syscall, __SYSCALL_NARGS_X(60, 0, 7, 6, 5, 4, 3, 2, 1, 0, ))(60, 0)
5. __SYSCALL_CONCAT(__syscall, 1)(60, 0)
6. __SYSCALL_CONCAT_X(__syscall, 1)(60, 0)
7. __syscall1(60, 0)

We can use this principle to implement a general CREDENTIAL macro that dispatches to the correct implementation macro based on the number of arguments passed.

Would you like to implement it?

[matheusmoreira]

Also I did not want to add the complexity of decoding the base32 string at runtime (although it is already handled similarly in the totp_face_lfs.c.)

Yeah. I did it that way because I looked at the LFS face and saw there was precedent for it. In that case it's necessary because of the parsing work but I didn't see much reason not to do it in the normal face as well. I want to eventually merge them both into one.

Like you I wished for a construct which would have allowed to decode it at compile time.

Your script is a fine solution. We just gotta integrate it with the build system somehow. Then we can make it generate a header that gets automatically included by the TOTP face. I haven't fully explored the makefiles yet but I will.

[maxz]

While exploring the musl codebase, I came across this wonderful piece of C preprocessor wizardry:

Nice find.

Would you like to implement it?

I'm not too keen on wrestling C preprocessor macros right now. Go ahead an implement it if you want to. If you don't, then I will probably get around to it in a few weeks.
I will just try to watch what you are doing to see whether you already implemented it. Or you could drop a line here if you did.

[matheusmoreira]

Go ahead an implement it if you want to.

Understood.

Or you could drop a line here if you did.

Sure thing.