Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add Whilelisting of callsigns; Enable disallow functionality #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fuelxc
Copy link

@fuelxc fuelxc commented Oct 3, 2020

Sorry about not having tests and stuff. I have no real experience with C so I was very very hacky. I dunno if someone can help me with the tests and cleaning it up.

@fuelxc
Copy link
Author

fuelxc commented Oct 3, 2020

I will note that I have live tested it:
config:

# Configuration for aprsc, an APRS-IS server for core servers

# Your unique server ID
ServerId  JEEPRSNET 

...

#
FileLimit        10000
AllowLoginCall KJ7QXU
DisallowLoginCall *
Escape character is '^]'.
# aprsc 2.1.8-gf07682dM
user KC7RZT pass 21927
# Login by user not allowed
user KJ7QXU pass 12345
# Login by unverified user not allowed
user KJ7QXU pass 22444
# logresp KJ7QXU verified, server JEEPRSNET
HP1CSL-9>PX5Q8Z,qAS,HP1UPR-1:`kKq+>/`"4o}Siguenos en @radioaficionpty_%

@snip
Copy link
Contributor

snip commented Oct 3, 2020

This feature looks very interesting!

@hessu
Copy link
Owner

hessu commented Oct 3, 2020

Hi, as you can see from the automatic test runs by Travis, the behaviour changes so that the existing tests fail. Please run the test suite (cd tests; make test) and fix any obvious issues. The .travis.yml in the root describes the steps to install dependencies for the tests.

In principle the feature looks good, but the PR should not break existing tests and use cases without careful consideration and good reasons to do so.

Copy link
Owner

@hessu hessu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the change described in the PR, the PR also disallows logins by unverified clients (wrong passcode). I don't think this change should not be done; and at the very least, it should be discussed a bit on a larger forum such as the APRSSIG first.


Separate multiple callsigns with spaces.

AllowLoginCall rejects logins by the specified callsign. Callsigns which
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an option which rejects logins by a specific callsign should be named "Disallow" or "Reject", instead of "Allow" which would imply the opposite.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, in fact this is documentation for an existing option, DisallowLoginCall. :)


AllowLoginCall P1RAT* P?ROT*

AllowSourceCall makes the server drop packets sent by the given source
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, DisallowSourceCall.

@@ -402,6 +437,13 @@ int login_handler(struct worker_t *self, struct client_t *c, int l4proto, char *
}
}


if (disallow_unverified && !(c->validated)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not right; this breaks connections from unverified clients. They are intentionally allowed to connect, they're just not allowed to send packets. There might be some yelling if this change is done as a surprise; if you wish to propose a change like this, please do so on APRSSIG.

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

Successfully merging this pull request may close these issues.

3 participants