SCION Control-Plane PKI tool

This PR adds scion-pki a tool to generate and renew keys, certificates, and TRCs used in the SCION control plane PKI.
Known issues:

Cert renewal missing
Tests missing

This change is

Reviewed 2 of 12 files at r1.
Review status: 2 of 12 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Reviewed 2 of 12 files at r1.
Review status: 4 of 12 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

go/tools/scion-pki/internal/keys/keys.go, line 46 at r1 (raw file):

	Short:     "Generate keys for the SCION control plane PKI.",
	Long: `
'keys' can be used to generate all the necessary keys used in the SCION control plane PKI.

It would be nice to also have it generate AS master keys, even though they are technically not in the PKI.

go/tools/scion-pki/internal/keys/keys.go, line 52 at r1 (raw file):

		Used to generate new keys.
	clean
		Used to remove all keys.

As this is not implemented, it would be nice to have (NOT IMPLEMENTED) in the help text. Same goes for any other unimplemented features.

go/tools/scion-pki/internal/keys/keys.go, line 58 at r1 (raw file):

		The root directory of all certificates and keys (default '.')
	-core
		Used with -all to generate core keys types as well.

Wait, -all doesn't generate core keys? That seems misleading.

go/tools/scion-pki/internal/keys/keys.go, line 60 at r1 (raw file):

		Used with -all to generate core keys types as well.
	-f
		Overwrite existing keys.

I'd move this above -core.

go/tools/scion-pki/internal/keys/keys.go, line 72 at r1 (raw file):

		Generate the AS offline root key.

The following selectors are available:

This i find to be a bit clumsy. It would be more intuitive (to me, at least) if you could specify like:
*-*
3-*
3-11

(but *-11 would be illegal). Then you don't need keywords, and it's pretty obvious what's happening.

go/tools/scion-pki/internal/keys/keys.go, line 110 at r1 (raw file):

	}
	subCmd := args[0]
	cmd.Flag.Parse(args[1:])

This is a bit weird - these flags have already been parsed by main. The simplest approach would be to not try to parse the subcommand flags in main, and handle it in the subcommands themselves. Is there a hidden gotcha with that approach? You end up with a small amount of code duplication, but at least the processing is done where you'd expect.
(I just spent a while wondering why the return value isn't used, and if so how errors are detected for subcmd flags).

Comments from Reviewable

Review status: 3 of 15 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

go/tools/scion-pki/internal/keys/keys.go, line 46 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It would be nice to also have it generate AS master keys, even though they are technically not in the PKI.

Done.

go/tools/scion-pki/internal/keys/keys.go, line 52 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
As this is not implemented, it would be nice to have (NOT IMPLEMENTED) in the help text. Same goes for any other unimplemented features.

Done.

go/tools/scion-pki/internal/keys/keys.go, line 58 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Wait, -all doesn't generate core keys? That seems misleading.

Well the tools doesn't know whether a given  AS is a core AS. It could infer it through the existence of a core-cert.ini file or by looking at trc.ini, however, this would make it so that the keys command couldn't be self-contained.

go/tools/scion-pki/internal/keys/keys.go, line 60 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd move this above -core.

Done.

go/tools/scion-pki/internal/keys/keys.go, line 72 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This i find to be a bit clumsy. It would be more intuitive (to me, at least) if you could specify like:
*-*
3-*
3-11

(but *-11 would be illegal). Then you don't need keywords, and it's pretty obvious what's happening.

Done.

go/tools/scion-pki/internal/keys/keys.go, line 110 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This is a bit weird - these flags have already been parsed by main. The simplest approach would be to not try to parse the subcommand flags in main, and handle it in the subcommands themselves. Is there a hidden gotcha with that approach? You end up with a small amount of code duplication, but at least the processing is done where you'd expect.
(I just spent a while wondering why the return value isn't used, and if so how errors are detected for subcmd flags).

The way flag.Parse works is it parses until it reaches the first positional argument, i.e., one without a leading - or --, thus we want process the command line in the way 'consume flags after scion-pki, 'consume command (trc/keys/certs etc)', 'consume flags after command', 'consume subcommand (gen, clean, verify, etc)', 'consume flags after subcommand', 'consume selector'. Each 'consume flagsis a call toflag.Parse()`.

Comments from Reviewable

Reviewed 7 of 18 files at r2.
Review status: 8 of 19 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

go/tools/scion-pki/README.md, line 23 at r2 (raw file):

The expected structure is the following:

/

Why tabs for indentation in this block? It'll make editing it painful.

go/tools/scion-pki/README.md, line 28 at r2 (raw file):

			AS1/
				cert.ini
				[core-cert.ini]

I'm not convinced by having separate INI files. Suggestion: have isd.ini and as.ini  files, each of which contain all the relevant information for that ISD/AS. That way you can just do a single scion-pki keys gen '*-*' and be done.

go/tools/scion-pki/README.md, line 38 at r2 (raw file):

		...

Thus, the first step is to generate the appropriate directory structure. Lets assume for this example

Line length.

go/tools/scion-pki/README.md, line 45 at r2 (raw file):

### Generating the keys

Now that we have the necessary directory structure in place (`keys` and `certs` directories will be generated

Line length.

go/tools/scion-pki/README.md, line 49 at r2 (raw file):

ASes.

`scion-pki keys gen -all -core 1-1; scion-pki keys gen -all -core 1-2; scion-pki keys gen -all -core 1-3`

The -all flag is a bit odd. Here's how i would expect a tool like this to work:

scion-pki keys gen 1-1: generate all keys for 1-1, error if any exist (preferably before creating any new files), or if -f is supplied overwrite all existing keys.
scion-pki keys gen -master 1-1: generate AS master key for 1-1
scion-pki keys gen -missing 1-1: generate all missing keys for 1-1.

-core would be made obsolete by my as.ini suggestion above. The INI file would tell the key gen if it should also create core keys.

go/tools/scion-pki/README.md, line 64 at r2 (raw file):

`scion-pki trc template 1-*`

to generate a template config file in `ISD1/trc.ini`. Now, we can adjust the values in `ISD1/trc.ini`

Line length.

go/tools/scion-pki/internal/keys/gen.go, line 47 at r2 (raw file):
Why are errors ok on a skipped dir?
Oh, crap:

Walk does not follow symbolic links.

That is too big of a restriction to tie ourselves to...

go/tools/scion-pki/internal/keys/keys.go, line 110 at r1 (raw file):
Previously, shitz wrote…
The way flag.Parse works is it parses until it reaches the first positional argument, i.e., one without a leading - or --, thus we want process the command line in the way 'consume flags after scion-pki, 'consume command (trc/keys/certs etc)', 'consume flags after command', 'consume subcommand (gen, clean, verify, etc)', 'consume flags after subcommand', 'consume selector'. Each 'consume flagsis a call toflag.Parse()`.

main() calls flag.Parse(), and then it calls cmd.Flag.Parse, and then this cmd.Flag.Parse is called. It's the latter two calls i'm referring to.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 1 at r2 (raw file):

// Copyright 2017 ETH Zurich

2018

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 46 at r2 (raw file):

// ProcessSelector processes the given selector and returns the top level directory
// to which the requested operation should be applied.
func ProcessSelector(option string, args []string, isdOnly bool) (string, error) {

args is not used (and doesn't seem to be relevant in any case).

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 96 at r2 (raw file):

func GetIAFromPath(path string) (*addr.ISD_AS, error) {
	match := iaRe.FindAllStringSubmatch(path, -1)

FindStringSubmatch is likely more suitable.

Comments from Reviewable

Review status: 5 of 19 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

go/tools/scion-pki/README.md, line 28 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'm not convinced by having separate INI files. Suggestion: have isd.ini and as.ini  files, each of which contain all the relevant information for that ISD/AS. That way you can just do a single scion-pki keys gen '*-*' and be done.

Done.

go/tools/scion-pki/README.md, line 38 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Line length.

Done.

go/tools/scion-pki/README.md, line 45 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Line length.

Done.

go/tools/scion-pki/README.md, line 49 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The -all flag is a bit odd. Here's how i would expect a tool like this to work:

scion-pki keys gen 1-1: generate all keys for 1-1, error if any exist (preferably before creating any new files), or if -f is supplied overwrite all existing keys.
scion-pki keys gen -master 1-1: generate AS master key for 1-1
scion-pki keys gen -missing 1-1: generate all missing keys for 1-1.

-core would be made obsolete by my as.ini suggestion above. The INI file would tell the key gen if it should also create core keys.

This is more or less done. The way it works now is basically -missing always.

go/tools/scion-pki/internal/keys/gen.go, line 47 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why are errors ok on a skipped dir?
Oh, crap:

Walk does not follow symbolic links.

That is too big of a restriction to tie ourselves to...

Changed to filepath.Glob based visiting

Comments from Reviewable

Reviewed 1 of 12 files at r1, 3 of 14 files at r3.
Review status: 9 of 19 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

go/tools/scion-pki/README.md, line 83 at r3 (raw file):

[Core AS Certificate]
CanIssue      = true

I could be wrong, but, isn't this defined by the nature of core vs non-core AS cert? Meaning that CanIssue = true would actually be illegal for a non-core AS, hence that setting shouldn't exist in the [AS Certificate] section. (The converse isn't true, though. A non-voting core AS would have CanIssue = false).

go/tools/scion-pki/internal/keys/keys.go, line 51 at r3 (raw file):

		The root directory of all certificates and keys (default '.')
	-f
		Overwrite existing keys.

Trailing whitespace.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 67 at r3 (raw file):

		}
	}
	absRoot, err := filepath.Abs(RootDir)

Why the absolute path?

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 85 at r3 (raw file):

		asDirs = append(asDirs, dirs)
	}
	return isdDirs, asDirs, nil

If you're trying to group the AS dirs by their ISD dir, it would be simpler to just return a map of ISDDir:ASDir.
That said, i'm not sure why you're dealing in directories at all. What you actually care about is the list of ISDs, and the list of ASes in each ISD. You can gather this information from globbing the FS, but why pass around dirs when you can trivially generate a dir from an IA? This leads to odd things like FilterASDirs. If you have a list of ASes, and you know which ones is cores, you don't need to pre-generate paths, they are available on demand.
Ah, i see you already have GetPath, which does exactly that. I would just use that a lot more.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 125 at r3 (raw file):

		panic(err)
	}
	return filepath.Join(absRoot, fmt.Sprintf("ISD%d/AS%d", ia.I, ia.A))

Why absolute paths? Relative to the RootDir is perfectly fine, and also a lot less confusing if there are symlinks involved.

Comments from Reviewable

Review status: 9 of 19 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):

		return err
	}
	return GenAll(filepath.Join(dir, "keys"), a.IsCore)

I don't think this is the right place to get the "is a core" var from, because now it's in two places, isd.ini, and as.ini. So, either we don't put the list of core ASes in isd.ini, or we don't have a core flag in as.ini. isd.ini feels cleaner, but, the [Core AS Certificate] section still needs to be in as.ini. So, maybe it's best to just say that as.ini is authoritative. Thoughts?

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):

		return err
	}
	return GenAll(filepath.Join(dir, "keys"), a.IsCore)

The function ordering is rather random. genKeys calls GenAll which is at the end of the file, which in turn calls GenKey which is directly after genKeys.

go/tools/scion-pki/internal/keys/gen.go, line 157 at r3 (raw file):

	if !core {
		return nil
	}

It would be good to warn if core keys exist for a non-core AS.

Comments from Reviewable

Reviewed 1 of 14 files at r3.
Review status: 10 of 19 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 113 at r3 (raw file):

		}
	}
	if err := ioutil.WriteFile(path, append(raw, "\n"...), perm); err != nil {

Warning: this will not fix permissions of an existing file. perm is only used for creating a new file. It might be safest to nuke the existing file.

Comments from Reviewable

Review status: 10 of 19 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

go/tools/scion-pki/README.md, line 47 at r3 (raw file):

The next step is to create all the necessary isd.ini files. We can generate templates with

`scion-pki trc template '1-*'`

It feels awkward that the isd.ini template is done by the trc command, and the as.ini template by the certs command. I'd suggest instead a tmpl command that is in charge of this.

go/tools/scion-pki/README.md, line 50 at r3 (raw file):

to generate a template config file in `ISD1/isd.ini`. Now, we can adjust the values in
`ISD1/isd.ini`until they look like this:

These values aren't a good match for isd.ini. 2/3rds of it is TRC-specific, but the rest is not. So, maybe
Isd = 1
Description = "Fancy ISD"

[TRC]
Version = 1
...

Hmm. I note that as.ini doesn't contain the ISD-AS, as it's implied by the dir. For the same reason, it probably doesn't make sense to have the ISD explicitly mentioned in isd.ini. Or else put the IDs in both, and enforce it.

Comments from Reviewable

Review status: 10 of 19 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

go/tools/scion-pki/README.md, line 23 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why tabs for indentation in this block? It'll make editing it painful.

Done.

go/tools/scion-pki/README.md, line 64 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Line length.

Done.

go/tools/scion-pki/README.md, line 47 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It feels awkward that the isd.ini template is done by the trc command, and the as.ini template by the certs command. I'd suggest instead a tmpl command that is in charge of this.

Completely agree. Done

go/tools/scion-pki/README.md, line 50 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
These values aren't a good match for isd.ini. 2/3rds of it is TRC-specific, but the rest is not. So, maybe
Isd = 1
Description = "Fancy ISD"

[TRC]
Version = 1
...

Hmm. I note that as.ini doesn't contain the ISD-AS, as it's implied by the dir. For the same reason, it probably doesn't make sense to have the ISD explicitly mentioned in isd.ini. Or else put the IDs in both, and enforce it.

Actually, all of the values are TRC specific. I was contemplating introducing a separate [TRC] section, but didn't see the use for it (at least for now). I removed the Isd config param.

go/tools/scion-pki/README.md, line 83 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I could be wrong, but, isn't this defined by the nature of core vs non-core AS cert? Meaning that CanIssue = true would actually be illegal for a non-core AS, hence that setting shouldn't exist in the [AS Certificate] section. (The converse isn't true, though. A non-voting core AS would have CanIssue = false).

That is true. A non-voting core AS would not have a core-AS certificate at all. I remove CanIssue entirely from as.ini.
The naming is really confusing, since a core-AS certificate is not the the certificate for a core-AS, but the certificate used to issue other AS certificates. We should actually think about renaming that, since things get really confusing for non-voting core-Ases.
In any case, afaics non-voting core ASes are exactly the same as non-core Ases from the perspective of scion-pki

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I don't think this is the right place to get the "is a core" var from, because now it's in two places, isd.ini, and as.ini. So, either we don't put the list of core ASes in isd.ini, or we don't have a core flag in as.ini. isd.ini feels cleaner, but, the [Core AS Certificate] section still needs to be in as.ini. So, maybe it's best to just say that as.ini is authoritative. Thoughts?

That's exactly why I put an explicit flag in as.ini (because [Core AS Certificate] has to be in there anyway). The core flag is generated from isd.ini, so if the user does not actively shoot himself in the foot, the isd.ini and the core flags in the as.inis are consistent which I think is fine.

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The function ordering is rather random. genKeys calls GenAll which is at the end of the file, which in turn calls GenKey which is directly after genKeys.

Done.

go/tools/scion-pki/internal/keys/gen.go, line 157 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It would be good to warn if core keys exist for a non-core AS.

Not sure if this should happen here, since you'd also want this check when generating certs etc. I'm thinking than an explicit consistency check subcommand would be us, e.g., scion-pki check '*-*'. I wouldn't to add it to this PR though.

go/tools/scion-pki/internal/keys/keys.go, line 51 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Trailing whitespace.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 1 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
2018

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 46 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
args is not used (and doesn't seem to be relevant in any case).

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 96 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
FindStringSubmatch is likely more suitable.

Function got removed.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 67 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why the absolute path?

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 85 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
If you're trying to group the AS dirs by their ISD dir, it would be simpler to just return a map of ISDDir:ASDir.
That said, i'm not sure why you're dealing in directories at all. What you actually care about is the list of ISDs, and the list of ASes in each ISD. You can gather this information from globbing the FS, but why pass around dirs when you can trivially generate a dir from an IA? This leads to odd things like FilterASDirs. If you have a list of ASes, and you know which ones is cores, you don't need to pre-generate paths, they are available on demand.
Ah, i see you already have GetPath, which does exactly that. I would just use that a lot more.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 113 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Warning: this will not fix permissions of an existing file. perm is only used for creating a new file. It might be safest to nuke the existing file.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 125 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why absolute paths? Relative to the RootDir is perfectly fine, and also a lot less confusing if there are symlinks involved.

Done.

Comments from Reviewable

Review status: 3 of 19 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

go/tools/scion-pki/internal/keys/keys.go, line 110 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
main() calls flag.Parse(), and then it calls cmd.Flag.Parse, and then this cmd.Flag.Parse is called. It's the latter two calls i'm referring to.

moving that into the commands and subcommands leads to A LOT of code duplication. At the start of each runCmd and runSubcmd method you have a block of code that parses cmdline flags, whereas now you only have it in main and in each runCmd function. Granted this comes at the cost of some clarity, since it's more intuitive to do it in runCmd and runSubcmd, but I'm not sure it's worth the trade-off

Comments from Reviewable

Reviewed 4 of 18 files at r4, 1 of 9 files at r5.
Review status: 7 of 20 files reviewed at latest revision, 17 unresolved discussions.

go/tools/scion-pki/README.md, line 50 at r3 (raw file):
Previously, shitz wrote…
Actually, all of the values are TRC specific. I was contemplating introducing a separate [TRC] section, but didn't see the use for it (at least for now). I removed the Isd config param.

I disagree that Description is TRC specific. I would say that description is, well, a description of the ISD that is used by the TRC, but is not specific to the TRC. In any case, having a [TRC] section would be really good, as otherwise when some value needs to be added that isn't for the TRC, the format has to be re-done.

go/tools/scion-pki/README.md, line 83 at r3 (raw file):
Proposal: change it to [Issuer Certificate].

In any case, afaics non-voting core ASes are exactly the same as non-core Ases from the perspective of scion-pki

Excellent point.

go/tools/scion-pki/README.md, line 47 at r4 (raw file):

The next step is to create all the necessary isd.ini files. We can generate templates with

`scion-pki tmpl isd '1-*'`

1-* doesn't make sense for an ISD. scion-pki tmpl isd should just take a single integer.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 43 at r5 (raw file):

// ProcessSelector processes the given selector and returns the top level directory
// to which the requested operation should be applied.

Outdated comment.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 65 at r5 (raw file):

		}
	}
	isdGlob := fmt.Sprintf("ISD%s", isdTok)

This is simple enough to do it in-line in the Glob call.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 73 at r5 (raw file):

	for _, dir := range isdDirs {
		base := filepath.Base(dir)
		isd, err := strconv.ParseUint(base[3:], 10, 12)

This 12 bothers me slightly. I suggest adding ISDBits and ASBits consts to addr/isd_as.go, and referencing them. Case in point: you're using 22 instead of 20 for parsing the AS number below.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 75 at r5 (raw file):

		isd, err := strconv.ParseUint(base[3:], 10, 12)
		if err != nil {
			return nil, common.NewBasicError("Invalid path", nil, "path", dir)

"Invalid path" is too generic. "Unable to parse ISD number from dir" , err, "path", dir) would be a lot better (and it includes the error). Same for parsing the AS below.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 78 at r5 (raw file):

		}
		asGlob := fmt.Sprintf("%s/AS%s", base, asTok)
		dirs, err := filepath.Glob(filepath.Join(RootDir, asGlob))

It would be slightly cleaner to do filepath.Glob(filepath.Join(dir, fmt.Sprintf("AS%s", asTok))). You do all the joining in a single pass (and using the stdlib to do it).

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 96 at r5 (raw file):

// FilterASDirs takes a list of paths and returns a list of paths corresponding to core ASes
// and a list of paths for all remaining ASes.

Outdated comment.

go/vendor/vendor.json, line 318 at r4 (raw file):

			"checksumSHA1": "4D8hxMIaSDEW5pCQk22Xj4DcDh4=",
			"path": "golang.org/x/crypto/hkdf",
			"revision": "94eea52f7b742c7cbe0b03b22f0c4c8631ece122",

All vendor entries for a single repo (golang.org/x/crypto/ in this case) must use the same commit id. Otherwise we're asking for chaos.

Comments from Reviewable

Reviewed 1 of 18 files at r4, 2 of 9 files at r5.
Review status: 10 of 20 files reviewed at latest revision, 15 unresolved discussions.

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):
Previously, shitz wrote…
That's exactly why I put an explicit flag in as.ini (because [Core AS Certificate] has to be in there anyway). The core flag is generated from isd.ini, so if the user does not actively shoot himself in the foot, the isd.ini and the core flags in the as.inis are consistent which I think is fine.

That isn't good enough, unfortunately. If an AS is demoted from core to non-core, and isd.ini is updated, but as.ini is not, now you have inconsistency. Having this information in only one place is critical for making sure that unexpected behaviour doesn't occur.

go/tools/scion-pki/internal/keys/gen.go, line 157 at r3 (raw file):
Previously, shitz wrote…
Not sure if this should happen here, since you'd also want this check when generating certs etc. I'm thinking than an explicit consistency check subcommand would be us, e.g., scion-pki check '*-*'. I wouldn't to add it to this PR though.

Yeah that sounds good.

go/tools/scion-pki/internal/keys/keys.go, line 110 at r1 (raw file):
Previously, shitz wrote…
moving that into the commands and subcommands leads to A LOT of code duplication. At the start of each runCmd and runSubcmd method you have a block of code that parses cmdline flags, whereas now you only have it in main and in each runCmd function. Granted this comes at the cost of some clarity, since it's more intuitive to do it in runCmd and runSubcmd, but I'm not sure it's worth the trade-off

As discussed on slack, this is good enough for now, and if we move to the https://github.com/mitchellh/cli lib then this becomes moot anyway.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 66 at r5 (raw file):

	}
	isdGlob := fmt.Sprintf("ISD%s", isdTok)
	isdDirs, err := filepath.Glob(filepath.Join(RootDir, isdGlob))

If there are no matches, this should exit with an error. Same for the AS glob.

Comments from Reviewable

Reviewed 3 of 18 files at r4, 1 of 9 files at r5.
Review status: 13 of 20 files reviewed at latest revision, 18 unresolved discussions.

go/tools/scion-pki/internal/conf/isd.go, line 30 at r5 (raw file):

)

const TrcConfFileName = "isd.ini"

This shouldn't be referred to as Trc any more. Same for the type, etc.

go/tools/scion-pki/internal/conf/isd.go, line 54 at r5 (raw file):

	} else {
		for _, ia := range t.CoreIAs {
			if ia.IAInt() == addr.IAInt(0) {

You can now do ia.IsZero(). But you should check that ia.I and ia.A are both non-zero, as either being zero is illegal: if ia.I == 0 || ia.A == 0 {

go/tools/scion-pki/internal/tmpl/isd.go, line 14 at r5 (raw file):

// See the License for the specific language governing permissions and
// limitations under the License.
package tmpl

There should be a blank line before package, any preceeding comment block is used as a docstring.

go/tools/scion-pki/internal/tmpl/tmpl.go, line 32 at r5 (raw file):

	Short:     "Generate configuration templates for ISDs and ASes.",
	Long: `
Insert long desc here.

:P

Comments from Reviewable

Reviewed 1 of 15 files at r3, 2 of 9 files at r5.
Review status: 15 of 20 files reviewed at latest revision, 25 unresolved discussions.

go/tools/scion-pki/internal/conf/as.go, line 91 at r5 (raw file):

		SubjectIA:     subject,
		Subject:       subject.String(),
		EncAlgorithm:  "curve25519xsalsa20poly1305",

These should be constants.

go/tools/scion-pki/internal/conf/as.go, line 100 at r5 (raw file):

}

type As struct {

This type should come before the Cert type.

go/tools/scion-pki/internal/conf/as.go, line 160 at r5 (raw file):

		return nil, err
	}
	as := &As{}

It looks like go-ini will treat embedded structs as separate sections, so i think you can replace a bunch of this code with just doing
ini.ReflectFrom(iniCfg, as)

I think the same applies to the current SaveTo.
https://github.com/go-ini/ini#other-notes-on-mapreflect for reference.

go/tools/scion-pki/internal/conf/as.go, line 186 at r5 (raw file):

}

func readSection(s *ini.Section) (*Cert, error) {

Bad name :P

go/tools/scion-pki/internal/conf/isd.go, line 38 at r5 (raw file):

	IssuingTime uint64
	Validity    uint64
	CoreIAs     []addr.IA `ini:"-"`

The handling here is a bit hacky, i think it can be made a bit cleaner by doing:
CoreASes  []addr.IA `ini:"-"`
RawCoreASes []string `init:"CoreASes"`

In LoadTrcConf you then do:
section.MapTo(...)
if len(t.RawCoreAses) == 0 {
  // error core ases missing
}
for _, rawIA := range t.RawCoreASes {
  ia, err := addr.IAFromString(rawIA)
  ...
   t.CoreASes = append(t.CoreASes, ia)
}

go-ini will already separate the value into strings for you, no trimming/splitting required.
The above also matches how we handle similar situations in proto/json.

go/tools/scion-pki/internal/conf/isd.go, line 43 at r5 (raw file):

}

func (t *Trc) validate() error {

Random function ordering strikes again :P

go/tools/scion-pki/internal/conf/isd.go, line 68 at r5 (raw file):

}

func (t *Trc) SaveTo(path string, force bool) error {

Write would be a more usual name in our codebase.

Comments from Reviewable

Reviewed 1 of 18 files at r4.
Review status: 16 of 20 files reviewed at latest revision, 29 unresolved discussions.

go/tools/scion-pki/internal/conf/as.go, line 45 at r5 (raw file):

	Issuer        string
	IssuerIA      addr.IA `ini:"-"`
	IssuingTime   int64

uint64.

go/tools/scion-pki/internal/conf/as.go, line 48 at r5 (raw file):

	TRCVersion    uint64
	Version       uint64
	Validity      uint64

I think it would be nice to use string here, and allow time.Duration formatted values, but it's more of a wishlist item.

go/tools/scion-pki/internal/trc/trc.go, line 50 at r5 (raw file):

		All ISDs under the root directory.
	X-*
		ISD X.

I don't think it makes sense for trc to use the AS-level selector.

go/tools/scion-pki/internal/trc/trc.go, line 71 at r5 (raw file):

the following values:
	Isd [required]
		integer representing the Isd ID for which the TRC is generated

Needs updating.

Comments from Reviewable

Review status: 16 of 20 files reviewed at latest revision, 29 unresolved discussions.

go/tools/scion-pki/internal/conf/as.go, line 48 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I think it would be nice to use string here, and allow time.Duration formatted values, but it's more of a wishlist item.

Ah. Currently the code assumes that this is an integer number of days. That's pretty coarse when you're dealing with a small number of days. E.g. the Issuing Cert expires in 1d23h, this would only allow you to get a new cert for 24h, instead of, say, 1d22h.

Comments from Reviewable

Reviewed 1 of 9 files at r5.
Review status: 17 of 20 files reviewed at latest revision, 30 unresolved discussions.

go/tools/scion-pki/internal/trc/gen.go, line 120 at r5 (raw file):

	// Sign the TRC.
	for _, as := range ases {
		if err := t.Sign(as.IA.String(), common.RawBytes(as.Online), crypto.Ed25519); err != nil {

Ah. This hard-codes that it is the online key being used. That's something that will need to be configurable at some point.

Comments from Reviewable

Reviewed 1 of 18 files at r4.
Review status: 18 of 20 files reviewed at latest revision, 34 unresolved discussions.

go/tools/scion-pki/internal/certs/certs.go, line 41 at r5 (raw file):

		Used to renew existing certificates.
	template
		Used to generate as.ini (core-as.ini if -core supplied) template configuration files.

Outdated.

go/tools/scion-pki/internal/certs/certs.go, line 43 at r5 (raw file):

		Used to generate as.ini (core-as.ini if -core supplied) template configuration files.
	clean (NOT IMPLEMENTED)
		Used to clean the PKI root directory.

root dir?

go/tools/scion-pki/internal/certs/certs.go, line 82 at r5 (raw file):

"AS Certificate" section and in case of core==true also a "Core AS Certificate" section
that can contain the following values:
	Subject [required]

I don't think this should be required, or even supported. The subject must be the local IA, which is already defined. I think the same applies to the Issuer for Issuer Certs.

go/tools/scion-pki/internal/certs/certs.go, line 94 at r5 (raw file):

		integer representing the version of the certificate
	Comment [optional]
		arbitrary string used to describe the certificate

Certificate, not AS?

go/tools/scion-pki/internal/trc/gen.go, line 120 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Ah. This hard-codes that it is the online key being used. That's something that will need to be configurable at some point.

Hurm. The signature type is also hard-coded. That's not great.

Comments from Reviewable

Review status: 18 of 20 files reviewed at latest revision, 38 unresolved discussions.

go/tools/scion-pki/internal/certs/gen.go, line 53 at r5 (raw file):

			base.ErrorAndExit("Error reading isd.ini: %s\n", err)
		}
		nonCores := pkicmn.FilterAses(ases, tconf.CoreIAs)

I'd move this to before the second loop, as that's where it's used.

go/tools/scion-pki/internal/certs/gen.go, line 74 at r5 (raw file):

	cpath := filepath.Join(dir, conf.AsConfFileName)
	if _, err = os.Stat(cpath); os.IsNotExist(err) {
		return nil

This should at least log that it's skipping a dir.

go/tools/scion-pki/internal/certs/gen.go, line 81 at r5 (raw file):

	}
	if core && a.CC == nil {
		return common.NewBasicError("'%s' section missing from as.ini", nil, "path", cpath)

'%s' is never evaluated.

go/tools/scion-pki/internal/conf/as.go, line 38 at r5 (raw file):

// Cert holds the parameters that are used to create certs.
type Cert struct {
	Comment       string

You can add comments via struct tags. E.g.:
*Embeded comment:"Embeded section"`` this would help a lot for describing the possible values of fields.

Comments from Reviewable

Review status: 18 of 20 files reviewed at latest revision, 39 unresolved discussions.

go/tools/scion-pki/internal/certs/gen.go, line 92 at r5 (raw file):

	}
	if err != nil {
		return common.NewBasicError("Error loading issuer cert", err, "subject", a.C.Subject)

This assumes the error was from the second case.

Comments from Reviewable

Reviewed 1 of 9 files at r5.
Review status: 19 of 20 files reviewed at latest revision, 44 unresolved discussions.

go/tools/scion-pki/internal/certs/gen.go, line 122 at r5 (raw file):

}

func genCertCommon(conf *conf.Cert, signKeyFname string) (*cert.Certificate, error) {

This should come after its callers.

go/tools/scion-pki/internal/certs/gen.go, line 134 at r5 (raw file):

		return nil, err
	}
	decKeyFixed := new([32]byte)

var decKeyFixed, decPub [32]byte
copy(decKeyFixed[:], decKey)
curve25519.ScalarBaseMult(&decPub, &decKeyFixed)

go/tools/scion-pki/internal/certs/gen.go, line 141 at r5 (raw file):

	issuingTime := uint64(time.Now().Unix())
	expirationTime := issuingTime + conf.Validity*24*60*60
	c := &cert.Certificate{

return &cert.Certificate{

go/tools/scion-pki/internal/certs/gen.go, line 181 at r5 (raw file):

	// Sign the certificate.
	// FIXME(shitz): The signing algorithm should be supplied or read from the TRC.
	if err = c.Sign(issuerKey, crypto.Ed25519); err != nil {

Can't you just use c.SignAlgorithm?

go/tools/scion-pki/internal/certs/gen.go, line 199 at r5 (raw file):

	// Ensure issuer can issue certificates.
	if !issuerCert.CanIssue {
		return nil, common.NewBasicError("Issuer cert not authorized to issue new certs.", nil,

s/new//, as it otherwise it sounds like it's valid to "issue" old certs (whatever that means).
Include the current subject.

Comments from Reviewable

Review status: 19 of 20 files reviewed at latest revision, 45 unresolved discussions.

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):

	Name:      "certs",
	Run:       runCert,
	UsageLine: "certs [-h] (gen|renew|template|verify|clean) [<flags>] [<selector>|<files>]",

This usage is incorrect. You can't supply a selector to verify (though i don't know why that wouldn't be supported), and you can't supply a file to gen, for example.
$ scion-pki certs verify '*-*'
Error reading *-*: open *-*: no such file or directory

Comments from Reviewable

Reviewed 1 of 9 files at r5.
Review status: all files reviewed at latest revision, 45 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 46 unresolved discussions.

go/tools/scion-pki/internal/certs/verify.go, line 39 at r5 (raw file):

		raw, err := ioutil.ReadFile(certPath)
		if err != nil {
			fmt.Fprintf(os.Stderr, "Error reading %s: %s", certPath, err)

Missing a trailing \n, in a number of other places too.

Comments from Reviewable

Review status: 7 of 21 files reviewed at latest revision, 47 unresolved discussions.

go/tools/scion-pki/README.md, line 50 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I disagree that Description is TRC specific. I would say that description is, well, a description of the ISD that is used by the TRC, but is not specific to the TRC. In any case, having a [TRC] section would be really good, as otherwise when some value needs to be added that isn't for the TRC, the format has to be re-done.

Done.

go/tools/scion-pki/README.md, line 83 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Proposal: change it to [Issuer Certificate].

In any case, afaics non-voting core ASes are exactly the same as non-core Ases from the perspective of scion-pki

Excellent point.

Done.

go/tools/scion-pki/README.md, line 47 at r4 (raw file):
Previously, kormat (Stephen Shirley) wrote…
1-* doesn't make sense for an ISD. scion-pki tmpl isd should just take a single integer.

Done.

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This usage is incorrect. You can't supply a selector to verify (though i don't know why that wouldn't be supported), and you can't supply a file to gen, for example.
$ scion-pki certs verify '*-*'
Error reading *-*: open *-*: no such file or directory

files is only valid together with verify. The problem with supporting the selector-based approach is specifying which version of a certificate should be verified. There are a couple of options:

By default verify the newest version only
Have an -all flag to verify all certificates for an AS
Have a -versions flag that specifies a list of versions to verify.

I intended certs verify to be a simple utility to verify specific, manually specify certs. Anything else I wouldn't want to do in this PR. I'm open for suggestions on how to improve the usage string.

go/tools/scion-pki/internal/certs/certs.go, line 41 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Outdated.

Done.

go/tools/scion-pki/internal/certs/certs.go, line 43 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
root dir?

Done.

go/tools/scion-pki/internal/certs/certs.go, line 82 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I don't think this should be required, or even supported. The subject must be the local IA, which is already defined. I think the same applies to the Issuer for Issuer Certs.

Done.

go/tools/scion-pki/internal/certs/certs.go, line 94 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Certificate, not AS?

Done.

go/tools/scion-pki/internal/certs/gen.go, line 53 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd move this to before the second loop, as that's where it's used.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 74 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should at least log that it's skipping a dir.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 81 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
'%s' is never evaluated.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 92 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This assumes the error was from the second case.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 122 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should come after its callers.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 134 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
var decKeyFixed, decPub [32]byte
copy(decKeyFixed[:], decKey)
curve25519.ScalarBaseMult(&decPub, &decKeyFixed)

Done.

go/tools/scion-pki/internal/certs/gen.go, line 141 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
return &cert.Certificate{

Done.

go/tools/scion-pki/internal/certs/gen.go, line 181 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Can't you just use c.SignAlgorithm?

No. c.SignAlgorithm is the signing algorithm that this core AS will use to sign other AS certificates (using core-sig.key). Here, online-root.key is used for which the signing algorithm is stored in the TRC.

go/tools/scion-pki/internal/certs/gen.go, line 199 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
s/new//, as it otherwise it sounds like it's valid to "issue" old certs (whatever that means).
Include the current subject.

Done.

go/tools/scion-pki/internal/certs/verify.go, line 39 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Missing a trailing \n, in a number of other places too.

Checked all Printf and Fprintf.

go/tools/scion-pki/internal/certs/verify.go, line 48 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
If any certs fail to verify (or there's some other error), the eventual exit code must be non-zero.

Done.

go/tools/scion-pki/internal/conf/as.go, line 38 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
You can add comments via struct tags. E.g.:
*Embeded comment:"Embeded section"`` this would help a lot for describing the possible values of fields.

Done.

go/tools/scion-pki/internal/conf/as.go, line 45 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
uint64.

Done.

go/tools/scion-pki/internal/conf/as.go, line 91 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
These should be constants.

Done.

go/tools/scion-pki/internal/conf/as.go, line 100 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This type should come before the Cert type.

Done.

go/tools/scion-pki/internal/conf/as.go, line 160 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It looks like go-ini will treat embedded structs as separate sections, so i think you can replace a bunch of this code with just doing
ini.ReflectFrom(iniCfg, as)

I think the same applies to the current SaveTo.
https://github.com/go-ini/ini#other-notes-on-mapreflect for reference.

Done.

go/tools/scion-pki/internal/conf/as.go, line 186 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Bad name :P

Gone.

go/tools/scion-pki/internal/conf/isd.go, line 30 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This shouldn't be referred to as Trc any more. Same for the type, etc.

Done.

go/tools/scion-pki/internal/conf/isd.go, line 38 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The handling here is a bit hacky, i think it can be made a bit cleaner by doing:
CoreASes  []addr.IA `ini:"-"`
RawCoreASes []string `init:"CoreASes"`

In LoadTrcConf you then do:
section.MapTo(...)
if len(t.RawCoreAses) == 0 {
  // error core ases missing
}
for _, rawIA := range t.RawCoreASes {
  ia, err := addr.IAFromString(rawIA)
  ...
   t.CoreASes = append(t.CoreASes, ia)
}

go-ini will already separate the value into strings for you, no trimming/splitting required.
The above also matches how we handle similar situations in proto/json.

Done.

go/tools/scion-pki/internal/conf/isd.go, line 43 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Random function ordering strikes again :P

Done.

go/tools/scion-pki/internal/conf/isd.go, line 54 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
You can now do ia.IsZero(). But you should check that ia.I and ia.A are both non-zero, as either being zero is illegal: if ia.I == 0 || ia.A == 0 {

Done.

go/tools/scion-pki/internal/conf/isd.go, line 68 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Write would be a more usual name in our codebase.

Done.

go/tools/scion-pki/internal/keys/gen.go, line 70 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
That isn't good enough, unfortunately. If an AS is demoted from core to non-core, and isd.ini is updated, but as.ini is not, now you have inconsistency. Having this information in only one place is critical for making sure that unexpected behaviour doesn't occur.

I remove the core flag from as.ini and only isd.ini is now used to determine whether an AS is core.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 43 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Outdated comment.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 65 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This is simple enough to do it in-line in the Glob call.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 66 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
If there are no matches, this should exit with an error. Same for the AS glob.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 73 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This 12 bothers me slightly. I suggest adding ISDBits and ASBits consts to addr/isd_as.go, and referencing them. Case in point: you're using 22 instead of 20 for parsing the AS number below.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 75 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
"Invalid path" is too generic. "Unable to parse ISD number from dir" , err, "path", dir) would be a lot better (and it includes the error). Same for parsing the AS below.

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 78 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It would be slightly cleaner to do filepath.Glob(filepath.Join(dir, fmt.Sprintf("AS%s", asTok))). You do all the joining in a single pass (and using the stdlib to do it).

Done.

go/tools/scion-pki/internal/pkicmn/pkicmn.go, line 96 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Outdated comment.

Done.

go/tools/scion-pki/internal/tmpl/isd.go, line 14 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
There should be a blank line before package, any preceeding comment block is used as a docstring.

Done.

go/tools/scion-pki/internal/tmpl/tmpl.go, line 32 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
:P

Done.

go/tools/scion-pki/internal/trc/gen.go, line 120 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Hurm. The signature type is also hard-coded. That's not great.

Hmm, I think this needs some change to isd.ini.  Basically, there should be a section for each core AS specifying the signing algorithms, e.g.,
Desc = ...

[TRC]
...
CoreASes = 1-1,1-2
...

[1-1]
OnlineKeyAlg = ed25519
OfflineKeyAlg = ed25519

If not specified, the default would be ed25519. If it's ok with you I would add this in a later PR, since it's a completely backwards-compatible extensions to isd.ini.
Whether to use the offline or online key should be a commandline flag imho.

go/tools/scion-pki/internal/trc/trc.go, line 50 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I don't think it makes sense for trc to use the AS-level selector.

Done.

go/tools/scion-pki/internal/trc/trc.go, line 71 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Needs updating.

Done.

Comments from Reviewable

Review status: 7 of 23 files reviewed at latest revision, 47 unresolved discussions.

go/tools/scion-pki/internal/conf/as.go, line 48 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Ah. Currently the code assumes that this is an integer number of days. That's pretty coarse when you're dealing with a small number of days. E.g. the Issuing Cert expires in 1d23h, this would only allow you to get a new cert for 24h, instead of, say, 1d22h.

Done.

Comments from Reviewable

Reviewed 1 of 15 files at r3, 1 of 18 files at r4, 18 of 18 files at r6.
Review status: 21 of 23 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

go/lib/util/duration.go, line 80 at r6 (raw file):

}

func WriteDuration(dur time.Duration) string {

FmtDuration would be a bit clearer. Write implies that it writes it to a specified place.

go/lib/util/duration.go, line 97 at r6 (raw file):

		"ms": int64(time.Millisecond),
		"us": int64(time.Microsecond),
		"ns": 1,

This can be int64(time.Nanosecond), for consistency.

go/lib/util/duration.go, line 118 at r6 (raw file):

		unit = "us"
	}
	return fmt.Sprintf("%v%v", ns/factors[unit], unit)

"%d%s" is preferred.

go/lib/util/duration_test.go, line 66 at r6 (raw file):

		isOk   bool
	}{
		{2 * time.Nanosecond, "2ns", true},

Add a test case for 0.

go/lib/util/duration_test.go, line 71 at r6 (raw file):

		{55 * time.Second, "55s", true},
		{66 * time.Hour, "66h", true},
		{48 * time.Hour, "48h", false},

I don't think this is needed - it's already covered by the next test. Same for 35d. In effect, i don't think you need the isOk handling.

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):
Previously, shitz wrote…
files is only valid together with verify. The problem with supporting the selector-based approach is specifying which version of a certificate should be verified. There are a couple of options:

By default verify the newest version only
Have an -all flag to verify all certificates for an AS
Have a -versions flag that specifies a list of versions to verify.

I intended certs verify to be a simple utility to verify specific, manually specify certs. Anything else I wouldn't want to do in this PR. I'm open for suggestions on how to improve the usage string.

Making it multi-line helps a lot:
certs (gen|renew|template|clean) [<flags>]  <selector>
certs verify [<flags>] <files>

go/tools/scion-pki/internal/certs/gen.go, line 87 at r6 (raw file):

	var issuerCert *cert.Certificate
	if core {
		issuerCert, err = genCoreASCert(a.IssuerCert, ia)

I think it would be good to rename this to genIssuerCert, to reflect the new naming. Actually, we should probably also rename core to issuer. That nicely isolates this part of the code from core vs non-signing core.

go/tools/scion-pki/internal/certs/gen.go, line 98 at r6 (raw file):

	}
	if issuerCert == nil {
		return common.NewBasicError("Issuer cert not found", err, "issuer", a.AsCert.Issuer)

I think err is not set in this case.

go/tools/scion-pki/internal/conf/as.go, line 77 at r6 (raw file):

	bc := NewTemplateCertConf(trcVer)
	a.AsCert = &AsCert{
		Issuer:   "0-0",

If core is true, this should be set to the local IA.

go/tools/scion-pki/internal/conf/isd.go, line 107 at r6 (raw file):

func (t *Trc) validate() error {
	if t.IssuingTime == 0 {
		t.IssuingTime = uint64(time.Now().Unix())

This isn't a good place to do this. The context in which t.IssuingTime should imply time.Now() is when you are writing a new TRC. If later code wants to check if there was an issuing time in the ini, it can't trust that the value hasn't just been written by the loading code. So, instead of handling it here, i suggest doing it in trc/gen.go. The same applies to certificates issuing time.
(The rest of the defaults are :+1: though).

go/tools/scion-pki/internal/conf/isd.go, line 127 at r6 (raw file):

	} else {
		for _, ia := range t.CoreIAs {
			if ia.I == 0 || ia.A == 0 {

ia.IsZero()

go/tools/scion-pki/internal/trc/gen.go, line 120 at r5 (raw file):
Previously, shitz wrote…
Hmm, I think this needs some change to isd.ini.  Basically, there should be a section for each core AS specifying the signing algorithms, e.g.,
Desc = ...

[TRC]
...
CoreASes = 1-1,1-2
...

[1-1]
OnlineKeyAlg = ed25519
OfflineKeyAlg = ed25519

If not specified, the default would be ed25519. If it's ok with you I would add this in a later PR, since it's a completely backwards-compatible extensions to isd.ini.
Whether to use the offline or online key should be a commandline flag imho.

Hmm. The as.ini would be a more natural place for the key algos, to me. But the rest sounds good in any case.

Comments from Reviewable

go/tools/scion-pki/README.md, line 38 at r7 (raw file):

        ...

Thus, the first step is to generate the appropriate directory structure. Lets assume for this

Let's

Comments from Reviewable

Review status: 21 of 23 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

go/lib/util/duration.go, line 80 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
FmtDuration would be a bit clearer. Write implies that it writes it to a specified place.

Done.

go/lib/util/duration.go, line 97 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This can be int64(time.Nanosecond), for consistency.

Done.:118

go/lib/util/duration.go, line 118 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
"%d%s" is preferred.

Done.

go/lib/util/duration_test.go, line 66 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Add a test case for 0.

Done.

go/lib/util/duration_test.go, line 71 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I don't think this is needed - it's already covered by the next test. Same for 35d. In effect, i don't think you need the isOk handling.

Done.

go/tools/scion-pki/README.md, line 38 at r7 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Let's

Good catch! Thanks! :100:

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Making it multi-line helps a lot:
certs (gen|renew|template|clean) [<flags>]  <selector>
certs verify [<flags>] <files>

Done.

go/tools/scion-pki/internal/certs/gen.go, line 87 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I think it would be good to rename this to genIssuerCert, to reflect the new naming. Actually, we should probably also rename core to issuer. That nicely isolates this part of the code from core vs non-signing core.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 98 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I think err is not set in this case.

Done.

go/tools/scion-pki/internal/conf/as.go, line 77 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
If core is true, this should be set to the local IA.

Done.

go/tools/scion-pki/internal/conf/isd.go, line 107 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This isn't a good place to do this. The context in which t.IssuingTime should imply time.Now() is when you are writing a new TRC. If later code wants to check if there was an issuing time in the ini, it can't trust that the value hasn't just been written by the loading code. So, instead of handling it here, i suggest doing it in trc/gen.go. The same applies to certificates issuing time.
(The rest of the defaults are :+1: though).

Done. Relatedly, I realized that certs/gen.go ignored the value from the configuration.

go/tools/scion-pki/internal/conf/isd.go, line 127 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
ia.IsZero()

That one checks if both I and A are 0. I think we want to check if either is zero here, since that is an invalid ISD-AS identifier. Actually, I added an IsValid() method to addr.IA to check this.

go/tools/scion-pki/internal/trc/gen.go, line 120 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Hmm. The as.ini would be a more natural place for the key algos, to me. But the rest sounds good in any case.

Yeah, as.ini is probably the better place.

go/vendor/vendor.json, line 318 at r4 (raw file):
Previously, kormat (Stephen Shirley) wrote…
All vendor entries for a single repo (golang.org/x/crypto/ in this case) must use the same commit id. Otherwise we're asking for chaos.

Done.

Comments from Reviewable

Reviewed 1 of 15 files at r3, 2 of 2 files at r7, 10 of 10 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

go/lib/addr/isdas.go, line 104 at r8 (raw file):

}

func (ia IA) IsValid() bool {

Not a fan of the name, see other comment.

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):
Previously, shitz wrote…
Done.

Hmm. The output is inconsistent now:
$ scion-pki certs
usage: certs [-h] (gen|renew|template|clean) [<flags>] <selector>
       scion-pki certs verify <files>
Run 'scion-pki help certs' for details.
$ scion-pki certs -h
usage: scion-pki certs [-h] (gen|renew|template|clean) [<flags>] <selector>
       scion-pki certs verify <files>
...

I think the problem is that the certs usage doesn't include the tool name, and that the help does.

go/tools/scion-pki/internal/certs/gen.go, line 84 at r8 (raw file):

	}
	fmt.Println("Generating Certificate Chain for", ia)
	// If we are core then we need to generate a core AS cert first.

Comment needs updating to issuer worldview.

go/tools/scion-pki/internal/certs/gen.go, line 134 at r8 (raw file):

	c.Issuer = s
	if c.Comment == "" {
		c.Comment = fmt.Sprintf("Core AS Certificate for %s version %d.", c.Subject, c.Version)

"Issuer Certificate for..."

go/tools/scion-pki/internal/conf/isd.go, line 127 at r6 (raw file):
Previously, shitz wrote…
That one checks if both I and A are 0. I think we want to check if either is zero here, since that is an invalid ISD-AS identifier. Actually, I added an IsValid() method to addr.IA to check this.

Oops, misread. I think IsValid isn't a good name, because it's entirely context dependent on whether a given value is valid or not. So, i think your original code was better, though i'm open to naming suggestions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 5 unresolved discussions.

go/lib/addr/isdas.go, line 104 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Not a fan of the name, see other comment.

Done.

go/tools/scion-pki/internal/certs/certs.go, line 30 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Hmm. The output is inconsistent now:
$ scion-pki certs
usage: certs [-h] (gen|renew|template|clean) [<flags>] <selector>
       scion-pki certs verify <files>
Run 'scion-pki help certs' for details.
$ scion-pki certs -h
usage: scion-pki certs [-h] (gen|renew|template|clean) [<flags>] <selector>
       scion-pki certs verify <files>
...

I think the problem is that the certs usage doesn't include the tool name, and that the help does.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 84 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Comment needs updating to issuer worldview.

Done.

go/tools/scion-pki/internal/certs/gen.go, line 134 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
"Issuer Certificate for..."

Done.

go/tools/scion-pki/internal/conf/isd.go, line 127 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Oops, misread. I think IsValid isn't a good name, because it's entirely context dependent on whether a given value is valid or not. So, i think your original code was better, though i'm open to naming suggestions.

Yes, you are right about the context dependency. I tried with ContainsZero or HasZero, but in the end didn't like it and just reverted to the previous version.

Comments from Reviewable

Reviewed 1 of 15 files at r3, 4 of 4 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


Saving Markdown Source from the MdEditor to a Local File

Describe the issue
Thank you for developing this library.
I am trying to save the Markdown source from the editor to a local file. However, the save button on the MdEditor does not seem to trigger any response.
Could you please guide me on how to achieve this?
Alternatively, is there another method to implement this functionality?
Thank you very much!
Procedure version
4.17.4
Reproduction link
No response

For the way to save content, you will need to use Blob-related technologies.
If you need to place a button on the toolbar, you can refer to this example.

For the way to save content, you will need to use Blob-related technologies.
If you need to place a button on the toolbar, you can refer to this example.

Thank you for your response.
Based on point 2 of your reply, does this mean that I cannot use the default save button and would need to place a new save button on the toolbar, bind a corresponding handler to it, and then use Blob-related technologies to implement the saving functionality?

No, you can use the default save button and save the text in the onSave event


Notebook UI - Adjusted cell spacing

This PR fixes #10746

Coverage decreased (-0.006%) to 34.209% when pulling 049c13122435aa6951c7dcba77d1a22f7aecde55 on ui-cell-spacing into 7b382794e257ce5d272b36a93f0ebe4ee024aebd on release/1.19.


Fix evidence string prefixing at source

In the ChEMBL evidence strings, the target.activity field is something like http://identifiers.org/cttv.activity/drug_positive_modulator, but this is mapped to just drug_positive_modulator by the data_pipeline (see here).
It would be better to ask providers to remove this and simplify our pipeline. There are several other instances in the data_pipeline that can be found by searching for [-1].

This also applies to other evidence sources. In a perfect world, we should use real ontology terms for these - not an ad-hoc controlled vocabulary of strings.

An overview of these terms can be found here.
It makes sense to use a controlled vocabulary. It might make sense to use existing ontology terms if we can and create the missing terms. There is a lot on for 18.12 already, so this will probably take a backseat until early next year.


cmd/compile: No BCE for x&(len(p)-1)

What version of Go are you using (go version)?

$ go version
go version go1.19 linux/amd64
$ gotip version
go version devel go1.20-9b8750f53e Sat Dec 10 00:35:24 2022 +0000 linux/amd64

What did you do?
Distilled from a custom hash table:
package ht

func get(p []string, h uint) string {
	if len(p) == 0 {
		return ""
	}
	i := h & uint(len(p)-1)
	return p[i]
}

What did you expect to see?
BCE for the expression p[i]. From the if statement, the compiler knows that len(p) > 0, so len(p)-1 is non-negative and in bounds, and so is h&(len(p)-1).
What did you see instead?
./bce.go:8:10: Found IsInBounds
It doesn't seem to matter if I put the cast on h or change the length check to <=0.

I would like to have a try on this. Maybe need a little bit changes on CL 419555

Just so you know, this is harder than it looks currently the facts table is filled in by OpCode specific knowlege first, then CFG is explored.
prove currently know how to handle everything in this example except the OpSub64 value.
It is rarely true that the result of a subtraction is always smaller or equal than the first input (it only happen if you prove that the second operand is always smaller or equal than the first one in the first place, such as in this case).
There are many ways you could solve this, I'm just saying this isn't a trivial thing like it might look like.


adding OWNERS file for CI/gating

This OWNERS file puts the repo more closely in line with our other repository openstack-helm, follows methods recommended by CNCF, and also follows our pattern for CI (as we do in openstack-helm). This will be added to both of the Halcyon repos.

LGTM


Data acquisition method

Thank you for your great work!
I have a question.
Is this dataset obtained by a human driving the robot manually?

Hi, Harin. Thanks for your attention to our work.
In our scenarios, each robot follows the designed trajectory by itself, while humans serve as reliable safeguards to handle variant road conditions.
Bests,
Dapeng


automate release with mud package

Our plain mud seems to be stuck at an old version? https://www.npmjs.com/package/mud

the experiment of releasing under a catchall package (mud) was aborted because yarn didn't support releasing the "monorepo" as package (by specifying different exports in the package.json, which I think would be the easiest way of creating implementing this catchall package, but happy for alternative approaches too). Now that we've switched to pnpm we might give it another try!


@jsonString includes the quotation marks used to mark the string/message

See title.
I'm writing a COMMAND_ALIAS for my tellraws. The goal is to have it formatted like so:
Name of Sender: Message
It works, but the "Name of Sender" includes the quotation marks when I use @jsonString, so it outputs like this:
"Name of Sender": Message
My command is as follows:
    {
        "commandMode": "COMMAND_ALIAS",
        "command": "tr {arg::minecraft:game_profile#Target} {arg::brigadier:string#FromName} {arg::minecraft:color#NameColor} {arg::minecraft:greedy_string#Message}",
        "execution": [
            {
                "command": "tellraw {Target} [\"\",{\"text\":\"{FromName@jsonString}\",\"color\":\"{NameColor}\"},\": {Message@jsonString}\"]",
                "type": "CLIENT"
            }
        ]
    },

So I do it like:
/tr @a "Satan" red Welcome to hell!
and it outputs:
"Satan": Welcome to hell!
But I want it as:
Satan: Welcome to hell!

Brigadier's string - Each string argument type can accept either a single word (no spaces), a quotable phrase (either single word or quoted string), or a greedy phrase (taking the rest of the command as the string argument).
So you can do /tr @a Satan red Welcome to hell!

You only quote /tr @a "If Satan's name has a space or more" red Welcome to hell! which will results in

Which is intended behavior from Brigadier. Perhaps I could add a functionality to remove quotes like @removeQuotes. I don't plan on updating COMMAND_ALIAS in the future, if you can you should move over to COMMAND_CUSTOM

Sorry, the quotations were happening yes when the name is in quotations to include spaces. I'm using it to make my NPCs talk, so the names are like "Guard A", "Guard B".

I have added a temporary branch with the @removeDoubleQuotes, you can fetch them from here. This will make it into the next official release but there is a format change.

Thanks! Will try it out. Can you combine 2 or more formatters?
Like {message@jsonString@removeDoubleQuotes}?

No(exact reason I'm abandoning COMMAND_ALIAS
for COMMAND_CUSTOM), I assume you just want to remove it from the name not the message.
    {
        "commandMode": "COMMAND_ALIAS",
        "command": "tr {arg::minecraft:game_profile#Target} {arg::brigadier:string#FromName} {arg::minecraft:color#NameColor} {arg::minecraft:greedy_string#Message}",
        "execution": [
            {
                "command": "tellraw {Target} [\"\",{\"text\":\"{FromName@removeDoubleQuotes}\",\"color\":\"{NameColor}\"},\": {Message@jsonString}\"]",
                "type": "CLIENT"
            }
        ]
    }
    ```

Just tested it out, @removeDoubleQuotes is working wonders. Finally was able to reduce the length of my tellraws. :) Thanks! Will wait for the next official release.


Chain multiple actions

Ability to chain more than one actuator in a rule. Like paint, sound, paint...

two basic options here

linear chaining (easiest, but then a lot of scrolling)
nested do sections (needs some UI to insert a second action), so a rule is like an "if (e) then { D1; D2, D3 }

I like the second option as you'll be able to see that a single has multiple (parallel actions)

Either way, you'll scroll. Iet's punt this for now, it does add complexity and maybe we don't need it.


hub.KerasLayer crashes Model.save() in h5 format

I'm consolidating various issues listed below to what I believe to be their common cause.
Problem:
In TF2 alpha, adding a hub.KerasLayer crashes Model.save() in h5 format with message "Unable to create link" from h5py. Hub authors need to follow up on ironing out that workflow.
How to repro (thanks @frogermcs):

Open https://colab.research.google.com/github/tensorflow/hub/blob/master/examples/colab/tf2_image_retraining.ipynb
Do one of

!pip install -U --pre tensorflow-gpu for 2.0.0-alpha0
!pip install -U --pre tf-nightly-gpu-2.0-preview for 2.0.0.dev20190503

!pip install tensorflow-hub for 0.4.0
At the very end, append a code block with the single line
model.save('path_to_my_model.h5')
Go "Runtime > Restart and run all"

For me (and the other reporters), that crashes with
/usr/local/lib/python3.6/dist-packages/h5py/_hl/group.py in __setitem__(self, name, obj)
    286             if isinstance(obj, HLObject):
--> 287                 h5o.link(obj.id, self.id, name, lcpl=lcpl, lapl=self._lapl)

h5py/_objects.pyx in h5py._objects.with_phil.wrapper()
h5py/_objects.pyx in h5py._objects.with_phil.wrapper()
h5py/h5o.pyx in h5py.h5o.link()
RuntimeError: Unable to create link (name already exists)

Issues that are likely subsumed by this:
tensorflow/hub https://github.com/tensorflow/hub/issues/255
https://github.com/tensorflow/tensorflow/issues/26835
https://github.com/tensorflow/tensorflow/issues/27688

Note workaround in
https://github.com/tensorflow/tensorflow/issues/26835#issuecomment-485534826

This appears to be caused by missing explicit variable names in the image modules tfhub.dev/google/tf2-preview/*/2. I'll see if re-exporting at head (i.e., past commit https://github.com/tensorflow/tensorflow/commit/e479cf6a28a8f0706328c796365c7a82a63c7b4e) helps.

I've just published
https://tfhub.dev/google/tf2-preview/inception_v3/feature_vector/3
https://tfhub.dev/google/tf2-preview/mobilenet_v2/feature_vector/3
and updated tf2_image_retraining.ipynb with a reference to them.
For me, that fixes the workflow from the initial comment (meaning, model.save('...h5') succeeds).

@arnoegw just ran the notebook and it is giving the following error when compiling:
`Building model with https://tfhub.dev/google/tf2-preview/mobilenet_v2/feature_vector/3
NotEncodableErrorTraceback (most recent call last)
 in ()
2 model = tf.keras.Sequential([
3     hub.KerasLayer(MODULE_HANDLE, output_shape=[FV_SIZE],
----> 4                    trainable=do_fine_tuning),
5     tf.keras.layers.Dropout(rate=0.2),
6     tf.keras.layers.Dense(train_generator.num_classes, activation='softmax',
11 frames
/usr/local/lib/python2.7/dist-packages/tensorflow_hub/keras_layer.pyc in init(self, handle, trainable, arguments, **kwargs)
96       self._func = handle
97     else:
---> 98       self._func = module_v2.load(handle)
99       if not callable(self._func):
100         raise ValueError("Non-callable result from hub.load('%s')" %
/usr/local/lib/python2.7/dist-packages/tensorflow_hub/module_v2.pyc in load(handle)
78                                 "format. Loading of the module using "
79                                 "hub.load() is not supported." % handle)
---> 80     return tf_v1.saved_model.load_v2(module_handle)
81   else:
82     raise NotImplementedError("hub.load() is not implemented for TF < 1.14.x, "
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in load(export_dir, tags)
322       loader = _Loader(object_graph_proto,
323                        saved_model_proto,
--> 324                        export_dir)
325       root = loader.get(0)
326   else:
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in init(self, object_graph_proto, saved_model_proto, export_dir)
55         function_deserialization.load_function_def_library(
56             meta_graph.graph_def.library))
---> 57     self._load_all()
58     # TODO(b/124045874): There are limitations with functions whose captures
59     # trigger other functions to be executed. For now it is only guaranteed to
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in _load_all(self)
130     node_setters = []
131     for proto in self._proto.nodes:
--> 132       node, setter = self._recreate(proto)
133       self._nodes.append(node)
134       node_setters.append(setter)
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in _recreate(self, proto)
193     if kind not in factory:
194       raise ValueError("Unknown SavedObject type: %r" % kind)
--> 195     return factorykind
196
197   def _recreate_user_object(self, proto):
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in ()
182         "user_object": lambda: self._recreate_user_object(proto.user_object),
183         "asset": lambda: self._recreate_asset(proto.asset),
--> 184         "function": lambda: self._recreate_function(proto.function),
185         "bare_concrete_function": functools.partial(
186             self._recreate_bare_concrete_function,
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/load.pyc in _recreate_function(self, proto)
217   def _recreate_function(self, proto):
218     return function_deserialization.recreate_function(
--> 219         proto, self._concrete_functions), setattr
220
221   def _recreate_bare_concrete_function(self, proto):
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/function_deserialization.pyc in recreate_function(saved_function, concrete_functions)
184   coder = nested_structure_coder.StructureCoder()
185   function_spec = _deserialize_function_spec(saved_function.function_spec,
--> 186                                              coder)
187
188   def restored_function_body(*args, **kwargs):
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/function_deserialization.pyc in _deserialize_function_spec(function_spec_proto, coder)
118       annotations=typeless_fullargspec.annotations)
119   is_method = function_spec_proto.is_method
--> 120   args_to_prepend = coder.decode_proto(function_spec_proto.args_to_prepend)
121   kwargs_to_include = coder.decode_proto(function_spec_proto.kwargs_to_include)
122   input_signature = coder.decode_proto(function_spec_proto.input_signature)
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/nested_structure_coder.pyc in decode_proto(self, proto)
112       NotEncodableError: For values for which there are no encoders.
113     """
--> 114     return self._map_structure(proto, self._get_decoders())
115
116
/usr/local/lib/python2.7/dist-packages/tensorflow/python/saved_model/nested_structure_coder.pyc in _map_structure(self, pyobj, coders)
69         return do(pyobj, recursion_fn)
70     raise NotEncodableError(
---> 71         "No encoder for object [%s] of type [%s]." % (str(pyobj), type(pyobj)))
72
73   def encode_structure(self, nested_structure):
NotEncodableError: No encoder for object [] of type [<class 'tensorflow.core.protobuf.struct_pb2.StructuredValue'>].`

Let's continue in the separate issue https://github.com/tensorflow/hub/issues/297 that you filed.


TagBot trigger issue

This issue is used to trigger TagBot; feel free to unsubscribe.
If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.
If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

Triggering TagBot for merged registry pull request: https://github.com/JuliaRegistries/General/pull/72484

TagBot fix

Triggering TagBot for merged registry pull request: https://github.com/JuliaRegistries/General/pull/72695


Russian localisation and small fixes in the Example

Also I've added .gitignore and pods libraries to the git (Because .workspace file presents in the git).

Hello @K-Be,
Before your code can be accepted into the project you must also sign the Contributor License Agreement (link below).
Also, please revert the angry birds bundle changes.
Autodesk Form Ind Contrib Agmt for ADAppRater.pdf
Thanx.
Amir

About licence, I'll  read it.
About bundle ID, can you select some bundle id for example project. I think is not clear to provide not complete example (without some correct bundle id example does not work).

I accept your approach and I'll insert our app's bundle.
I just don't think it is ok to use an unrelated third party's bundle for that matter.

Sorry, what is  "our app's bundle"?
Do you talk about "com.autodesk.apprater.demo" bundle id? With this bundle id I can't run example to exam library. Or do you talk about some other your product?

Some other product

Ok, write here bundle id and I'll change it.

com.autodesk.autocadws

I've sent the signed agreement.


Encrypt all sensitive keys

Probably should have a way to encrypt the private keys and api keys on the PC running Maize and then decrypt them using the passcode/passphrase that encrypted them when you enter the app. Just so they aren't sitting in plain text on the computer. LooPyGen has an option like this I believe.

This is a great idea since the user no longer needs to touch the file itself.

next release (v1.11.0) will have account management in it. user will have to enter in a password to log in. there will also be two keys that are stored in the application that will be hard to get to (why the user password is necessary). the source code for the encryption keys and process will not be open source.
thanks for the push @fudgebucket27 ^_^

Awesome mate! Feel free to close this out as you've implemented it now 😊


Similar example with Flutter with Ivs broadcast

Hallo, I am building a solution in flutter for my client for ivs streaming along with stage, i would like to know if we can achieve this Flutter and if you can give me a ref.
Thanks in Advance.

Hello @rahulkumar96t, unfortunately I am not aware of any similar examples using Flutter. Since this issue is not directly related to the demo, I will close it. However, I encourage you to post your question on AWS re:Post in case someone there has an answer for you.


Data Science with Python : Neural Networks Cheatsheet #1849

Description 📜
I did an extensive documentation and prepared a cheatsheet on Neural Networks, and how it's applied using sample code. Added one MD and one IPYNB files.
Fixed #1849

Type of change 📝

[x] Documentation (Content Creation in the form of codes or tutorials)

Domain of Contribution 📊

[x] Datascience with Python

Checklist ✅

[x] I follow Contributing Guidelines & Code of conduct of this project.
[x] I have performed a self-review of my own code or work.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] My changes generates no new warnings.
[x] I'm GWOC'21 contributor

Screenshots / Gif (Optional) 📸

@prathimacode-hub please check


Wrong NHK BS HD Link

It seems that the link of the NHK BS stream is written on the llst as:
https://vnpt.nekocdn.xyz/9ca1151d-7a96-4c1a-9672-8853124bba85,m3u8
When it actually should be:
https://vnpt.nekocdn.xyz/9ca1151d-7a96-4c1a-9672-8853124bba85.m3u8

Pull request proposed:
https://github.com/luongz/iptv-jp/pull/79


apistar cant work and the commandline cant recognized from 0.4.1 above

and for 0.4.0, when enter apistar it will have the error like :
Traceback (most recent call last):
File "c:\users\williamchen\anaconda3\Lib\runpy.py", line 193, in _run_module_as_main
"main", mod_spec)
File "c:\users\williamchen\anaconda3\Lib\runpy.py", line 85, in run_code
exec(code, run_globals)
File "C:\Users\williamchen\Envs\mxonline\Scripts\apistar.exe_main.py", line 5, in
ImportError: cannot import name 'main'

and the same happen in the centos 7, apistar cant be found ...

As per the release notes...

The apistar command line tool has not yet been re-introduced.

Run API Star applications directly instead, as-per the example in the quickstart.


Keyboard is not popping up in Firefox

1) Keyboard isn't popping up on second tap.
I am using TextFormField in Flutter web. It works as expected in Google Chrome. But when it is launched in Firefox, the keyboard doesn't pop up properly.
When clicked for the first time:

Works great.
 But if I focus on a different TextFormField, then it'll work fine.

It works fine in Google Chrome.
2) Problem while focusing on the text in TextFormField.
Also, when there are multiple lines in the TextFormField, as shown above, I am able to focus on the second line in Chrome but I can only focus on the first line in Firefox. Sorry, I don't think screenshots will be of much use to show this error.
3) The cursor isn't rendering properly.
There's one more issue with how the cursor is rendered in Chrome. I wasn't able to figure it out in Fiefox as it wasn't rendering at all.

I launched the web app in the following phone:
Device: Mi POCO F1
Android Vesion: 9
Browser:
Google Chrome (V80.0.3987.119)
Mozilla Firefox (V68.5.0)

Hi @melwinlobo18
what flutter version do you use?

Flutter version 1.14.6
Channel beta

/cc @nturgut @mdebbar

Thanks for filing the issue. Can you please use the master channel and let us know if it is working for you?

I switched the channel.
Flutter version 1.16.2-pre.23
Channel master
The problems which I mentioned above still exist. Along with it, I noticed a few more issues while selecting text from the TextFormField.
Google Chrome

Mozilla Firefox

I launched the web app in the following phone:
Device: Mi POCO F1
Android Vesion: 9
Browser:
Google Chrome (V80.0.3987.119)
Mozilla Firefox (V68.5.0)

@melwinlobo18 thanks a lot for filing the issue.
It looks like there are different issues here:
(1) Firefox Virtual keyboard, we can use this issue for it.
(2) Browser cursor is visible for multiline fields. It looks like this happens both in Chrome and Firefox. Can you create another issue for this one?
(3) Text measurement @mdebbar is this related to the other text measurement issues?

I raised this issue a few months ago. I had forgotten about it. But today I opened Hack20 and realized these issues still exist. Especially the Firefox Virtual keyboard issue, I have to reload the page for it work properly again. I hope this issue will be fixed soon.
@nturgut I'll file a new issue for Browser cursor is visible for multiline fields :)

Hi @melwinlobo18
Are you still experiencing this issue on latest master
If yes, Can you please provide your flutter doctor -v and a minimal complete reproducible code sample
Thank you

Hi @melwinlobo18
Unable to reproduce this issue
Without additional information, we are unfortunately not sure how to resolve this issue.
We are therefore reluctantly going to close this bug for now.
Please don't hesitate to comment on the bug if you have any more information for us; we will reopen it right away!
Thanks for your contribution.


Recyclable Iterators

I would like to introduce the idea that fastutil should support recyclable iterators.  That is, when calling the iterator() method, the same iterator object should be returned but reset to the start.
When trying to avoid allocations, it's clear that escape analysis [can] (http://psy-lob-saw.blogspot.ca/2014/12/the-escape-of-arraylistiterator.html) remove these allocations, but that means nothing when trying to profile the application, all I can see is these iterators because escape analysis is turned off by the profiler so I just fallback to using elements() where I can.
As long as the collections are not being paired with a lock, it seems like this is pretty straightforward.  For the synchronized collections, this might be complicated.  Although a lock must be held for the duration of the iteration, the iterator itself might be retained beyond the scope of the lock, and then be used again when the lock is re-acquired

Interesting idea.
I think this should not be the default behavior of iterator(); if desired, it should be on a separate method, like fastIterator() is; possibly recycledIterator().
Even in single-threaded applications, multiple iterators can be active over the same object, e.g. in nested loops processing pairs of items in a list or set. Quietly breaking such code by violating the iterator() contract would be a very bad thing.

I think this should not be the default behavior of iterator()

If you make another method, the standard for loop won't be able to call it.  It might warrant another set of generated classes

How about somehow providing the iterator from outside? This way, it
would be possible to reuse the same iterator for iterating multiple
collections.

This feature would be awesome. But @mdekstrand is correct. It should not be the default behavior of iterator() due to the unforseen amount of multithreading errors

You could add a new constructor which will either use the default iterator or recycled iterator by default with the warning that once such collection is created one behavior or the other applies for all iterators of such instance.

Or another option is to generate recycled iterator variants of each collection.

I think a better option would be to have a reset() (or something) method that resets an iterator. Returning always the same object is too big a break from the current practice (and from the specification).

@vigna yes that's a much better option, basically you reuse the iterator rather than breaking the JCF specs.

I have had this on my wishlist for some time, but I don't think this should be handled by the collection.
Instead, iterators should simply have a reset() function. I'd assume this is fairly easy to implement for all iterators on collections.
Then you could simply get the iterator once, and reset() it before each use. With multiple threads, you can get a separate iterator for each thread, or even handle an "iterator pool" of iterators if necessary (similar to how people have been doing database connection pools).

Maybe "rewind()", and we could have a RewindableIterator interface.
On June 24, 2017 12:19:11 AM GMT+02:00, Erich Schubert<EMAIL_ADDRESS>wrote:

I have had this on my wishlist for some time, but I don't think this
should be handled by the collection.
Instead, iterators should simply have a reset() function. I'd assume
this is fairly easy to implement for all iterators on collections.
Then you could simply get the iterator once, and reset() it before
each use. With multiple threads, you can get a separate iterator for
each thread, or even handle an "iterator pool" of iterators if
necessary (similar to how people have been doing database connection
pools).
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/vigna/fastutil/issues/45#issuecomment-310785391

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Since fastutil already adds skip(int), is it really necessary to add a RewindableIterator interface? Why not just add rewind() to drv/Iterator.drv, with a default implementation throwing an exception like remove()?

Well, an interface has the advantage that people can easily check for the feature. What you propose implies that to check one has to try and check the exception with a very complicated logic...

To make it usable, we need the return value to be a RewindableIterator, and at the same time a primitive-specific iterator. So this would mean introducing RewindableFloatIterator etc.; and as far as I can tell we will probably not have a non-rewindable float iterator anywhere in fastutil.

Just as an example of how it could look like, Agrona always returns the same iterator instance on its collections and calls reset() when getting the iterator (example here).

That violates the Map contract.

@jheusser I think that is a really bad idea.
What if I need a nested loop, to compute pairwise similarities? I need to be able to get two different iterators...
I like the idea of rewind(), but iterator recycling needs to be done by the user (you could try to use finalizer/reference magic, but that will likely be more expensive than new iterators!)
With the reset() discussed above, one could do
IntIterator i1 = col.iterator(), i2 = col.iterator();
while(i1.hasNext()) {
  int first = i1.nextInt();
  i2.reset();
  while(i2.hasNext()) {
    int second = i2.nextInt();
    // Do something on (frist, second)
  }
}

Note that in this example, I need the iterator to have both rewind() and nextInt().

@kno10 @vigna just highlighting how another high-performance library does it. Clearly, Agrona is not meant as drop-in replacement for standard collections but for specific use cases.
I do like the explicit rewind() approach as well.

Why was this closed?
Is there an option to reuse/rewind an iterator now?

Why was this closed?
Is there an option to reuse/rewind an iterator now?

Nope, but this was from 5 years ago and no one proposed a PR. I have to clean up issues once in a while or I lost track of what's urgent...

Nope, but this was from 5 years ago and no one proposed a PR. I have to clean up issues once in a while or I lost track of what's urgent...

+1 for explicit rewind() reset()


Collect team members' ORCIDs for citation

@ProjectPythia/core, over in #388 @jukent is working on creating a citation file for Foundations. We would like to collect ORCID id numbers for all team members so we can all receive appropriate authorship credit for the book.
Please add a comment or review to #388 with your ORCID. If you don't have one, now is a great time to create one!

Here's mine: https://orcid.org/0000-0003-2907-038X

Here is mine: https://orcid.org/0000-0002-0370-8974

Here you go: https://orcid.org/0000-0003-2788-9017

0000-0001-7246-6502 - Thanks y'all!

These have all been merged in #388


feat: implement JUMP instruction

Pull Request type

Please check the type of change your PR introduces:

[x] Feature

What is the current behavior?

Resolves: #250
What is the new behavior?

Does this introduce a breaking change?

[ ] Yes
[ ] No

Opened: https://github.com/kkrt-labs/kakarot-ssj/issues/353

done


refactor: price ticker

Summary
To test:

set ARKSCAN_NETWORK_CAN_BE_EXCHANGED=true in the .env file
set ARKSCAN_NETWORK=production in the .env file
run php artisan explorer:cache-currencies-data

Checklist

[ ] I checked my UI changes against the design and there are no notable differences
[ ] I checked my UI changes for any responsiveness issues
[ ] I checked my UI changes in light AND dark mode
[ ] I checked my (code) changes for obvious issues, debug statements and commented code
[ ] I opened a corresponding card on Clickup for any remaining TODOs in my code
[ ] I added a short description on how to test this PR (if necessary)
[ ] Documentation (if necessary)
[ ] Tests (if necessary)
[ ] Ready to be merged

https://app.clickup.com/t/861mrtzjj


Avatar WebRTC setup on page load instead of session start click

Hi,
I'm trying to embed the avatar in an HTML web page. Using the basic.html example file, I'm able to load the avatar without problems using the "Start Session" button. But then, if I want to force the session start of the avatar at the page loaded event (instead of button click) the Avatar doesn't appear, although all loading phases and logs are ok.

Could anyone suggest me how to investigate more thoroughly?
Thank you.
Gabriele

@yinhew  could you pleas take a look?

Hi, @ggiunta92
Please refer to https://developer.mozilla.org/en-US/docs/Web/Media/Autoplay_guide
This is an industry policy which most browsers are following.
You need some user interaction before auto-playing some media.
Before your change, the click on the button is the user interaction.
After your change, the user interaction is gone.
That's the reason.
Thanks,
Yinhe

Hi, @yinhew
perfect understood. Thanks for your support.
Gabriele


Rename files/variables etc from MyProject/myproject etc

Hi Matt,
I'm on the MPHYG002 course and made this little script to make it easier to start new projects with your template by renaming all the instances of MyProject/myproject/the namespace etc.
Having run the renaming script, CMake builds without complaining on my machine (macOS Sierra,  AppleClang <IP_ADDRESS>00042). I can't vouch for whether it would work on linux!
Cheers,
Danial

Thanks for this. I'll take a look.

Hi Danial,
Would you be happy to remove your copyright statement, and place the standard LICENSE info at the top of the rename.sh file?
If so, please can you provide a commit with this in?
Thanks
Matt

Hi Matt,
Have pushed that to the rename branch, so feel free to merge it in.
Cheers,
Danial
On 28 Feb 2017, at 08:36, Matt Clarkson <notifications<EMAIL_ADDRESS>wrote:
Hi Danial,
Would you be happy to remove your copyright statement, and place the standard LICENSE info at the top of the rename.sh file?
If so, please can you provide a commit with this in?
Thanks
Matt
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com/MattClarkson/CMakeCatchTemplate/pull/1#issuecomment-282976335, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIKgK3uo8C3AZdNjKiqbeptDMJXlkUo2ks5rg9yfgaJpZM4LsI_G.

Thanks for your contribution. This was manually merged, and manually updated.


Support .Publish(...)

For reference, the Rx Design Guidelines 5.10 suggest to use this operator.
The operator .publish() returns an ConnectableObservable, this means you first need to subscribe all your sources, .connect() it, keep a reference to your connect to prevent leaks, and then you also end up with a hot observable, so downstream subscribers better be ready as well.
Most of the time you only want to duplicate a stream locally. RxJS 4 and many other Rx implementations allow you to write
xs.publish(ob => ob.zip(ob.skip(1), (x, y) => {...}))

Here we use publish to peek at the next item in the source. Another favorite of mine is
xs.publish(ob => ob.window(ob.debounce(100)))

Because often you want to use a source for its own window-boundary.
This operator is very similar to .let, but avoid subscribing to the source twice. Useful on cold observables, but also to avoid the overhead of subscribing multiple times to a hot source. (imagine you use this trick 4x in a row with let, you end up with 2^4th subscriptions.)

Would you elaborate bit more? so far I know current publish already returns ConnectableObservable (https://github.com/ReactiveX/rxjs/blob/34cdef5a3d652c75a546e5bd73733fd21dc64fc0/src/operator/publish.ts#L15) would you mind to share some snippet of pusedo for suggested behavior?

The operator publish([selector]) should not return an ConnectableObservable, instead it returns the result of creating the connectable observable, passing the connectable observable to the selector,
aka
function publish(source, selector) {
  return Rx.Observable.create(ob => {
    var xs = source.publish();
    return selector(xs).subscribe(ob).add(xs.connect());
  });
}

Similar to publish in RxJS 4 and Rx.Net.

selector
Type: System.Func<IObservable, IObservable>
The selector function which can use the multicasted source sequence as many times as needed, without causing multiple subscriptions to the source sequence.

Thanks for clarification. I actually noticed RxJS 5 missing publish accepts selector compare to RxJS 4, and wasn't able to look up reason. @staltz , @blesh , remember there was discussions to align sharing operators in RxJS 5, is this conclusion of it or it's yet migrated from RxJS 4?

Yeah, I think this makes sense to implement.

I'll try to introduce those changes.


Mocha change

Mocha
Before

## After

(the second one needs some work)
The rest will come with future commits (its like 1 second)

I have some problems with latte so the update will be later on

Thanks for the PR! :)
Can we have the red text + icons in the first image changed to catppuccin red? thanks again
Closes #3 btw

btw the centered text isn't present anymore

ok sure :)

nvm, its impossible

I will do the images later

I'll do it tomorrow

Fixed in 29da571