Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs/install.md: formatting correction #675

Closed
wants to merge 1 commit into from
Closed

Conversation

ia
Copy link

@ia ia commented Jan 12, 2024

Hello. I'm sorry for removing the template of Pull-Request form.

But this is just a tiny markdown syntax correction for docs/install.md which I noticed while was reading all available documents before putting my hands on this interesting project.

P.S. I did sign my soul to Google CLA agreement by specifying my GitHub account in accordance with this document (in case if it's really required here and/or for any other contribution to any other Google-related open source project).

@kaczmarczyck kaczmarczyck self-requested a review January 12, 2024 12:32
@kaczmarczyck
Copy link
Collaborator

Hi, thank you for your interest and contribution! You can treat the pull request template as a reminder, and if it doesn't apply, no problem.
Since the CLA workflow passes, it looks like you did all necessary steps to contribute to Google projects and your offering was accepted.

I am curious why our Markdown linter workflow didn't catch this before. In fact, if I locally run

mdl **/*.md -c .markdownlint.json

I see a long list of minor lints. But even that doesn't find the bug that this PR fixes.
I will follow this up with new PR that fixes our Markdown, and maybe the workflow if I find the problem. Thanks for bringing this to my attention.

kaczmarczyck
kaczmarczyck previously approved these changes Jan 12, 2024
@kaczmarczyck kaczmarczyck changed the base branch from 2.1 to develop January 12, 2024 12:56
@kaczmarczyck kaczmarczyck dismissed their stale review January 12, 2024 12:56

The base branch was changed.

@kaczmarczyck kaczmarczyck requested a review from ia0 January 12, 2024 12:56
@kaczmarczyck kaczmarczyck changed the base branch from develop to 2.1 January 12, 2024 12:57
kaczmarczyck
kaczmarczyck previously approved these changes Jan 12, 2024
ia0
ia0 previously approved these changes Jan 12, 2024
@kaczmarczyck
Copy link
Collaborator

By the way, we usually accept PRs against develop, and then eventually we split a numbered release out of develop. So preferably, you could rebase it to develop. Otherwise I'll include it in my own PR.

@ia
Copy link
Author

ia commented Jan 12, 2024

Hi, thank you for your interest and contribution!

Thank you & other contributors to make FIDO2/2FA more accessible with this project!

You can treat the pull request template as a reminder, and if it doesn't apply, no problem.

Got it.

Since the CLA workflow passes, it looks like you did all necessary steps to contribute to Google projects and your offering was accepted.

Yeah, and I noticed that there is the linter even for that. Amazing.

I am curious why our Markdown linter workflow didn't catch this before. In fact, if I locally run

mdl **/*.md -c .markdownlint.json

I see a long list of minor lints. But even that doesn't find the bug that this PR fixes. I will follow this up with new PR that fixes our Markdown, and maybe the workflow if I find the problem. Thanks for bringing this to my attention.

Interesting. I haven't got time myself yet to check all active linters & checkers in this project, but now I'm curious to look into that mdl checker too. If I have time & I will find something, I will let know.

@ia
Copy link
Author

ia commented Jan 12, 2024

By the way, we usually accept PRs against develop, and then eventually we split a numbered release out of develop. So preferably, you could rebase it to develop. Otherwise I'll include it in my own PR.

Aha... well, I was debating with myself on that, so thanks for pointing this out! But right now I'm literally on the go (just didn't want to leave your feedback unattended out of courtesy), so let me try to do that just using GitHub interface and I hope it will do the trick. Otherwise I will do it "today" but later.

P.S. By the way, since we're here... I did get the "writing on the wall" that this project is experimental, not production ready and even from the point of theoretical & practical cryptography is not fully "safe" yet (like that mention about non-constant time). But out of curiosity: does anyone from Google employees use it with real tokens on a daily regular basis to login/authenticate for not-so-important resources? Just a "yes"/"no" question basically, thanks :)

@ia ia changed the base branch from 2.1 to develop January 12, 2024 14:04
@ia ia dismissed stale reviews from ia0 and kaczmarczyck January 12, 2024 14:04

The base branch was changed.

@ia ia changed the base branch from develop to 2.1 January 12, 2024 14:08
@kaczmarczyck
Copy link
Collaborator

I realized that the workflow runs this linter:
https://github.com/igorshubovych/markdownlint-cli

I ran it, and the linter mdl, and fixed all outputs that apply. I updated the config for our workflow in #676 as well. Interestingly, your bug is found by neither. Maybe that is because it is a syntax problem, not a lint.

@ia
Copy link
Author

ia commented Jan 12, 2024

let me try to do that just using GitHub interface and I hope it will do the trick.

Oh, right... my install-md have been "branched" not from develop so it doesn't work like that.

Can I just make a separate proper PR for develop branch instead of messing with git?

@kaczmarczyck
Copy link
Collaborator

You can, but I was faster :) See #676. Feel free to open another PR if you find anything else!

@kaczmarczyck
Copy link
Collaborator

I realized I overlooked your question here:

P.S. By the way, since we're here... I did get the "writing on the wall" that this project is experimental, not production ready and even from the point of theoretical & practical cryptography is not fully "safe" yet (like that mention about non-constant time). But out of curiosity: does anyone from Google employees use it with real tokens on a daily regular basis to login/authenticate for not-so-important resources? Just a "yes"/"no" question basically, thanks :)

I hope you don't mind an answer that is not just yes or no.

This repository has the CTAP logic inside libraries/opensk and also implements the environment layer for the Nordic chip in src/env/tock. But it is not too hard to use OpenSK on any other chip that is supported by Tock, and certainly possible to implement an environment for other chips.

Our CTAP logic has been FIDO certified for branch 2.0, and I recently tested against their compatibility tool for 2.1 out of curiosity and we are passing. We are still changing the environment API from time to time, but the CTAP logic is not experimental in a sense that it's less safe than other security keys.

Running OpenSK on the Nordic chip with software cryptography does have this downside though, that a local attacker could try side channel attacks etc. Whether or not this is part of your attacker model is up to you.

Now, to finally answer your yes or no question: I have an OpenSK based security key on my desk that I use for real life login.

@ia
Copy link
Author

ia commented Jan 13, 2024

I hope you don't mind an answer that is not just yes or no.

No-no, not at all! Just to be clear - I added that disclaimer of mine about yes/no question only to save you time for something more important, but thank you so much for taking your time with such a detailed response. It was super interesting! Sincerely appreciate it.

[...] I have an OpenSK based security key on my desk that I use for real life login.

Awesome!

P.S. Just one more question out of curiosity about organization of this project on GitHub: if develop branch is the main upstream branch here, then why 2.1 release branch is marked as the default one? Shouldn't develop marked as default? Thanks in advance.

@kaczmarczyck
Copy link
Collaborator

develop is the main branch for developers, and the numbered branches are for users. They went through more end-to-end tests, like trying different boards and operating systems, CTAP test tools etc. A new PR on develop might break something from time to time (even though CI should prevent the worst).

So we either have to try to send contributors to develop, or change the main branch and send users to the latest numbered branch. For my choice, see the README ;)

If you want to contribute, go to the develop branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants